Skip to content

[5.4] Fix b/c breaking change in AbstractView::get#45949

Closed
heelc29 wants to merge 5 commits intojoomla:5.4-devfrom
heelc29:5.4/fix-45702
Closed

[5.4] Fix b/c breaking change in AbstractView::get#45949
heelc29 wants to merge 5 commits intojoomla:5.4-devfrom
heelc29:5.4/fix-45702

Conversation

@heelc29
Copy link
Copy Markdown
Contributor

@heelc29 heelc29 commented Aug 20, 2025

Alternative Pull Request to #45940 .

Summary of Changes

Guided tour set fields variable to empty array and this was checked incorrectly in #45702

?? 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

  • No documentation changes for docs.joomla.org needed
  • No documentation changes for manual.joomla.org needed

@exlemor
Copy link
Copy Markdown

exlemor commented Aug 21, 2025

I have tested this item ✅ successfully on 56818f4

I have tested this successfully! Thanks @heelc29!


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/45949.

@brianteeman
Copy link
Copy Markdown
Contributor

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.

@richard67
Copy link
Copy Markdown
Member

@heelc29 Could you check and update your testing instructions? They tell about the ternary shortcut ?:, but your code does not use that, it uses a ternary with !empty() condition (which is right, I think).

@cybersalt
Copy link
Copy Markdown

I have tested this item ✅ successfully on 56818f4

Tested this and it worked.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/45949.

@joomdonation
Copy link
Copy Markdown
Contributor

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.

@richard67
Copy link
Copy Markdown
Member

@joomdonation What do you suggest? Revert the original PR completely, like it would have been done with PR #45940 ?

@richard67 richard67 added the RMDQ ReleaseManagerDecisionQueue label Aug 23, 2025
@brianteeman
Copy link
Copy Markdown
Contributor

Clearly that PR was wrong

@joomdonation
Copy link
Copy Markdown
Contributor

@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.

@richard67
Copy link
Copy Markdown
Member

@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.

@joomdonation
Copy link
Copy Markdown
Contributor

@richard67 If so, revert original PR would work.

@brianteeman
Copy link
Copy Markdown
Contributor

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

@joomdonation
Copy link
Copy Markdown
Contributor

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

@heelc29
Copy link
Copy Markdown
Contributor Author

heelc29 commented Aug 23, 2025

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

That's why I asked @Hackwar for a comment (see description of original PR #45702)
as negative side effect #44162 the access to protected properties of the view class is no longer possible. Is that acceptable?

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.

I am also convinced that some code is marked as deprecated but is still used 100 times by the core.
Example CurrentUserTrait #45700

If so, revert original PR would work.

Let me know how to proceed from here.

@richard67
Copy link
Copy Markdown
Member

I think for 5.4 we have to play safe and revert the initial PR #45702 completely.

@brianteeman Could you re-open your PR #45940 for that? If that is not possible, either make a new PR or let me know and I make one. Thanks in advance.

@brianteeman
Copy link
Copy Markdown
Contributor

as it was closed by @chmst I do not have the access level required to re-open

@richard67
Copy link
Copy Markdown
Member

as it was closed by @chmst I do not have the access level required to re-open

@brianteeman Thanks for restoring the branch. I've re-opened the PR. Made 2 CS suggestions.

@richard67 richard67 added the bug label Aug 26, 2025
@richard67
Copy link
Copy Markdown
Member

For this PR here we (maintainers) will discuss if we will use it for 6.0-dev.

@richard67
Copy link
Copy Markdown
Member

We (release managers and other maintainers) have decided to completely revert the b/c break in 5.4 and 6.0 for the upcoming beta 2 releases, and so we decided for PR #45940 , and this one here is obsolete for now.

It would have worked for the core, but might not have been sufficient for all 3rd party extension issues.

@heelc29 Thanks a lot for your efforts working on the issue. I's much appreciated.

Closing in favour of #45940 .

@richard67 richard67 closed this Aug 28, 2025
@heelc29 heelc29 deleted the 5.4/fix-45702 branch November 1, 2025 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug PR-5.4-dev RMDQ ReleaseManagerDecisionQueue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants