Skip to content

[6.0] Fix incorrect language tag comparison #45947

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

Open
wants to merge 6 commits into
base: 6.0-dev
Choose a base branch
from

Conversation

Fedik
Copy link
Member

@Fedik Fedik commented Aug 20, 2025

Pull Request for Issue #45941 and #45945 .

Summary of Changes

Fix language tag comparison when short tag from URI was compared with full tag.

Testing Instructions

It tricky to test in the core.
Add dd('test'); after
https://github.com/joomla/joomla-cms/blob/ea20ffd58058bc4fce3e547ab30a7d7903369306/plugins/system/sef/src/Extension/Sef.php#L113-L112

And open /index.php?option=com_content&view=article&id=6&lang=en on testing installation.

Actual result BEFORE applying this Pull Request

PHP warnings.

Expected result AFTER applying this Pull Request

No PHP warnings

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

@Hackwar please check, maybe the correct full tag should be already set in parseRule?

@exlemor
Copy link

exlemor commented Aug 20, 2025

@Fedik Thanks for the discovery and fix. I was going to pass it as successful but then I discovered a really odd situation... let me explain...

This testing installation (which I recently re-installed) did NOT have the System Configuration setting:
Force HTTPS turned on... (Entire Site)

During testing I found that WITH your patch on and the dd('test'); text added to line 113, I could not turn the site to HTTPS... I would get:

HTTPS has not been enabled as it is not available on this server. HTTPS connection test failed with the following error: HTTPS version of the site returned an invalid HTTP status code.

I have tried this now 5 times so I doubt it's my testing method/process. I also have other test installations on the same domain in other sub-folders and none of them have an issue with Enabling/Disabling HTTPS...

Any ideas as this relates? is it important/worrysome or just because of the specific testing done for this PR?

Thanks.

@joeforjoomla
Copy link
Contributor

@Fedik Yes good catch, this seems to fix the problem

@joeforjoomla
Copy link
Contributor

I have tested this item ✅ successfully on 89b46af


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

@exlemor
Copy link

exlemor commented Aug 20, 2025

@Fedik Yes good catch, this seems to fix the problem

(sorry for my question now but does that mean what I found is nothing to be concerned about, i.e. that I can validate your PR as is or am I waiting for an update from your to re-test and where I should not come across the same thing (HTTPS not being able to be turned on)?

Thanks for your patience - just trying to make sure @Fedik

@Fedik
Copy link
Member Author

Fedik commented Aug 20, 2025

@exlemor yes it is tricky, the testing code dd('test'); will break any redirect,
I think what you get is kind of fine, but do no worry. We still be need review from @Hackwar

@brianteeman
Copy link
Contributor

Do you know which pr introduced this bug? We should check everything in that pr

@Fedik
Copy link
Member Author

Fedik commented Aug 20, 2025

@Fedik
Copy link
Member Author

Fedik commented Aug 20, 2025

Here still a question, should the full tag be already set in the parseRule method?
Hard to compare with old behavior.

@exlemor
Copy link

exlemor commented Aug 20, 2025

I have tested this item ✅ successfully on 89b46af

I have tested it successfully...

That said, I also found this artifact/quirk discussed here:
#45947 (comment)

based on @Fedik's replies and comments and an upcoming future review by Hackwar, I will validate my test.


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

@Fedik
Copy link
Member Author

Fedik commented Aug 20, 2025

Do not set it RTC for now, I still need to check few things.

@Fedik
Copy link
Member Author

Fedik commented Aug 21, 2025

Okay, should be good now,
I checked Joomla 5 does not set lang in to Uri for full tag, and I update the pr to work the same.

@exlemor
Copy link

exlemor commented Aug 21, 2025

I have tested this item ✅ successfully on cde2124

I have retested this successfully. Thanks @Fedik!

(the HTTPS quirk that I mentioned in my earlier test is still present if it matters).


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

@exlemor
Copy link

exlemor commented Aug 23, 2025

I have tested this item ✅ successfully on 9742ae1

I have re-tested this successfully during PBF 23.08. Thanks @Fedik!


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

@richard67
Copy link
Member

@joeforjoomla Could you test this PR again? Thanks in advance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug PBF Pizza, Bugs and Fun PR-6.0-dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[6.0 Beta] Regression language filter plugin
6 participants