-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[5.4] Fix b/c breaking change in AbstractView::get #45949
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 5.4-dev
Are you sure you want to change the base?
Conversation
I have tested this item ✅ successfully on 56818f4 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/45949. |
I have zero confidence in this PR. The first version had three breaking changes. Two were spotted before it was merged. A third was only spotted after it was merged. |
@heelc29 Could you check and update your testing instructions? They tell about the ternary shortcut |
I have tested this item ✅ successfully on 56818f4 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/45949. |
I just had a quick look at the original PR and saw that it has a potential issue. You had to change some class properties to public like this one https://github.com/joomla/joomla-cms/pull/45702/files#diff-c0ddd6d01f117db59d8e37f1e2a6b44d1ac9718e480790c5d25650fbfa939043R38 mean third party extensions have this property defined as protected will be broken The deprecation to AbstractView::get method is also questionable. It prevents access to private or protected data of view classes in the layouts (causes the change you had to made in that PR), so I even unsure if that deprecation is valid. |
@joomdonation What do you suggest? Revert the original PR completely, like it would have been done with PR #45940 ? |
Clearly that PR was wrong |
@richard67 We will need to discuss first to see if we really want to deprecate AbstractView::get method before making further decision. If we still want to deprecate that method, the the completely revert is not needed but we will have to fix all backward incompatible changes. |
@joomdonation Well, the deprecation says it will be removed in 7.0, so we could revert the initial PR, which would mean 5.4 and 6.0 would still use deprecated code, not nice but allowed. That would give us more time to decide about the deprecation, if we remove it or only move it from 7 to 8. Beta 2 is not far away, so we might not have the time for long discussions. |
@richard67 If so, revert original PR would work. |
Only not nice if the depreciation was valid in the first place. To me it's something that was done without any consideration of the consequences which we are now seeing |
To me, the depreciation was not very well thought. I understand that we want to call model methods directly to get data and pass it to the view but we didn't care about the case it is used to allow access to none public property in the class https://github.com/joomla/joomla-cms/blob/5.3-dev/libraries/src/MVC/View/AbstractView.php#L175-L177 |
That's why I asked @Hackwar for a comment (see description of original PR #45702)
I am also convinced that some code is marked as deprecated but is still used 100 times by the core.
Let me know how to proceed from here. |
Alternative Pull Request to #45940 .
Summary of Changes
Guided tour set fields variable to empty array and this was checked incorrectly in #45702
joomla-cms/administrator/components/com_guidedtours/tmpl/tour/edit.php
Line 92 in 55951c4
??
is the null coalescing operator. It checks if a variable is set and not null. → An empty array ([]) is not null, so it will be returned.?:
is the ternary shortcut. It checks if a value is truthy. → An empty array is considered falsy, so the fallback value will be used.Testing Instructions
Open a guided tour or guided tour step for editing and go to the publishing tab
Actual result BEFORE applying this Pull Request
The tab contains no fields
Expected result AFTER applying this Pull Request
The tab contains publishing related fields
Link to documentations