Skip to content
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

[5.3] Improve System - Language Filter plugin code #44841

Merged
merged 3 commits into from
Feb 9, 2025

Conversation

joomdonation
Copy link
Contributor

Pull Request for Issue # .

Summary of Changes

This PR improves System - Language Filter code:

  • Add missing $event parameter to event listener methods. Also add void return type declaration to these methods (all event listener methods should have void return type)
  • Introduce $app and $language variables to avoid repeat calling methods to get these objects
  • Fixed a small bug with onPrivacyCollectAdminCapabilities does not return data properly. It needs to use $event->addResult to return data instead of return command. I will backport this bug fix to 5.2 if required

Testing Instructions

  • Expect to have at least one code review from maintainer
  • Setup a multilingual website using Joomla 5.3 nightly build, apply patch, navigate to random pages from frontend of your site and make sure nothing is broken.

Actual result BEFORE applying this Pull Request

Works

Expected result AFTER applying this Pull Request

Works, code is better. A small bug is fixed.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@Fedik Fedik added the Feature label Feb 9, 2025
@Fedik
Copy link
Member

Fedik commented Feb 9, 2025

I have tested this item ✅ successfully on f54e47a


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

@richard67
Copy link
Member

richard67 commented Feb 9, 2025

Hmm, with this PR applied, all was working during my tests, but I got a PHP warning which I do not get without this PR applied:

PHP Warning:  Attempt to read property "sef" on null in /home/richard/lamp/public_html/joomla-cms-5.3-dev/plugins/system/languagefilter/src/Extension/LanguageFilter.php on line 278

I get this when using the button in the backend for going to the frontend or when using the site URL without any language suffix appended, i.e. the pure domain only.

Update: And I get it only when having this PR here applied and in addition PR #44844 for the debug plugin. Weird.

Update: Ot happens also when I apply only PR #44844 but not this one here. Will report in PR #44844 .

@richard67
Copy link
Member

I have tested this item ✅ successfully on f54e47a


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

@richard67 richard67 removed the Feature label Feb 9, 2025
@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 9, 2025
@laoneo laoneo enabled auto-merge (squash) February 9, 2025 16:06
@laoneo laoneo added this to the Joomla! 5.3.0 milestone Feb 9, 2025
@laoneo laoneo merged commit b2cae37 into joomla:5.3-dev Feb 9, 2025
3 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 9, 2025
@joomdonation joomdonation deleted the improve_system_language_filter branch February 11, 2025 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants