[#46341] - [5.4] Fix condition to check request format in MenusHelper - deprecation warning
- Ready to Commit
- Medium
- Build: 5.4-dev
- # 46341
- Diff
- MacJoom:MacJoom-url-entry-deprecation-warning
User tests: Successful: Unsuccessful:
Pull Request for Issue #45707
Summary of Changes
Check for index.php? instead of index.php before entering parse_url
Testing Instructions
As in described in the issue:
1/ Create a menu item "URL" type - enter just index.php
2/ Set errors report to maximum in global config
3/ A warning shows up in the menu item (backend)
Actual result BEFORE applying this Pull Request
Deprecated: parse_str(): Passing null to parameter #1 ($string) of type string is deprecated in /var/www/clients/client2/web46/web/administrator/components/com_menus/src/Helper/MenusHelper.php on line 76
Expected result AFTER applying this Pull Request
No warning
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
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Administration com_menus |
| Title |
|
||||||
I have tested this item 🔴 unsuccessfully on f8d1efdThis does remove the notices but the link does not work Any url in the format index.php/link will not work (make sure you test from a page that is not the home page
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/46341.
Yes i see - but this is yet another bug - happens without the patch as well - only happens if you do use things like index.php/team works if you use only /team
We could just replace index.php/xxx with /xxx
urls in the format index.php/xxx are created when you enabled SEF urls (the default) and do not enable URL rewriting (the default)
Can we just pass this PR that fixes the original issue and open a new issue for the index.php/ bug. Needs to be fixed somewhere else. In the router i guess
I have tested this item ✅ successfully on f8d1efd
I have tested this successfully. Love to see Deprecation messages fixed! :)
Thanks @MacJoom!
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/46341.
I have tested this item ✅ successfully on f8d1efd
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/46341.
Can we just pass this PR that fixes the original issue and open a new issue for the index.php/ bug. Needs to be fixed somewhere else. In the router i guess
@brianteeman Could you check and possibly change your test result?
It fixes the deprecation but doesn't fix the URL. For me there's no point in fixing one without the other.
It fixes the deprecation but doesn't fix the URL. For me there's no point in fixing one without the other.
@brianteeman But does that justify an unsuccessful test result? The PR doesn't claim to fix the URL.
for me its all related. It might be if the url is fixed so that it works then this PR is redundant
I have tested this item ✅ successfully on f8d1efd
For me, with the URL set to index.php and the patch applied the link always reloaded the current page. The current page path is being appended after index.php/ - I would be inclined to treat what it is supposed to do as a separate issue.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/46341.
| Status | Pending | ⇒ | Ready to Commit |
| Labels |
Added:
bug
PR-5.4-dev
|
||
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/46341.
On further investigation this PR is not needed, because entering just index.php/xzy is wrong or at least not intented behaviour.
We should make clear that entering index.php/xyz in a System Links URL works only at home page level. on a subpage 'index.php/xyz ' is relative to the subpage and will fail accordingly. The correct usage would be /index.php/xyz - in which case this PR is not needed. Every System Link URL should start with http, https or / - i don't think we should fix user error. May be some message in the Backend helps
we should validate the link and warn with appropriate message
Also I just tested on a site with htaccess sef enabled and it works perfectly without the / in that scenario
| Labels |
Added:
RTC
|
||
As a little help from the Bugsquad: We discussed this in our meeting and would approve this PR. We think this is the correct fix. Yes, there is more to do in the longterm, but for now it would be a valid fix.
I have tested this item 🔴 unsuccessfully on f8d1efd
This does remove the notices but the link does not work
Any url in the format index.php/link will not work (make sure you test from a page that is not the home page
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/46341.