User tests: Successful: Unsuccessful:
Pull Request to fix frontend form behaviour #42989.
The PR #42989 has unfortunately revealed an incorrect behaviour in conjunction for the routing for the forms in frontend. With the enabled strict routing option of the sef plugin, all redirects that can be set for actions like cancel, save and similar controller functions no longer work correctly as the menu parameters can no longer be transferred.
Reason is the missing connection to the assigned menu item as the url is not generated correctly.
This repesents a B/C break that was certainly not intended.
However, it is highly likely that this will also affect some third-party extensions.
Make sure the option for strict routing is set to yes in Plugins: System - SEF.
First Test: Create article redirect
Second Test: Login redirect
Create a menu item -> type: Login Form
Set the options for redirecting for login and logout
Result for the testing instructions:
Redirect options are ignored and the final redirect always goes to the homepage.
Result for the testing instructions
Redirect options work and you will be redirected to the correct page after the action.
//cc @Hackwar
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 |
Category | ⇒ | Front End com_config com_contact com_content com_users |
I have tested this item ✅ successfully on ba6f7a8
Tested in com_content + code review
Labels |
Added:
Release Blocker
bug
PR-5.3-dev
|
This is not the correct fix. The right fix would be to remove the whole hard coded URL and instead use Route::_('index.php')
instead.
Updated with the proposed fix from @Hackwar and additionally tested for user tasks - login, logout, password reset, registration and username reminder request.
If you find the time I would be very grateful if you could redo your tests. @chmst @coolcat-creations
Nothing has changed in the testing instructions, everything should lead to the same result again.
it would be great if we had some consistency with the ordering of the parts of the form settings. in the long run it makes it easier to spot missing parts.
I would have done this as a separate PR to 5.4 to avoid polluting this PR and because its not a bug fix BUT as these are all views which might have overrides I wouldnt want to ask the user to check all their overrides on 5.3.x and 5.4
So please can we update this PR with changes to provide consistency in the order of parts
I would suggest
<form action="<?php echo Route::_('index.php'); ?>" method="post" id="application-form" name="adminForm" class="form-validate">
I have tested this item ✅ successfully on 07f17a5
Tested successfully. Thank you for your work!
@LadySolveig Now after you have implemented the fix suggested by @Hackwar , does the "(band-aid only)" in the title still fit? Or is it a real fix now? And could you check and if not too much work implement @brianteeman 's suggestion #45619 (comment) ?
The attributes are now sorted consistently. 3fa4508 Many thanks to Brian for this suggestion.
I have adapted it slightly, as fewer changes to existing files were necessary.
The order is now as follows
Would you be so kind as to restore the test from webnet-assmann, as nothing has functionally changed. @richard67
Can you briefly explain what the plan could be @Hackwar
If I have understood you correctly, it definitely needs documentation for third-party developers, but the router behaviour remains as it is now, right? I definitely need your support for documentation. I'm not deep enough in the topic to be able to document it properly.
Then I can adjust the title and description as Richard requested.
Cypress test for the frontend edit would now unfortunately also have to be checked. My guess is that the test has already been written on the wrong behaviour. But I haven't had time to look at it yet.
May I ask you if you could validate this for me if you have time @muhme
The router indeed stays the way it is, since that has been the behavior of this for literally decades. I have no idea why someone thought it might be a good idea to add the option part to the URL. It only worked because of the falsely added Itemid and I have the feeling that this was also part of the reason why this behavior was introduced in the first place. Simply said: If you want to send a request to the current URL, then you have to use the current URL and you get that by simply handing in index.php
. That will handle everything for you. You then only have to add additional POST parameters as hidden input fields, like you had to do before. This would also drop all the need for hidden Itemid
input fields... (at least for component views. Modules might be different, for example the mod_finder module)
Title |
|
If you want to send a request to the current URL, then you have to use the current URL and you get that by simply handing in index.php. That will handle everything for you. You then only have to add additional POST parameters as hidden input fields, like you had to do before. This would also drop all the need for hidden Itemid input fields... (at least for component views. Modules might be different, for example the mod_finder module)
This sounds horrible. index.php is normally the start page not current page and if we have different behaviours depending where we are (component, module) it sounds like chaos. So when I want to link to a different view I have to do magic instead of just linking to the component?
We need to get this pr merged asap to overcome the broken behaviour in core (thanks for the fix @LadySolveig ) but if we copy a form somewhere else and its behaviour totally changes should ring all alerts!
(For current URL I would expect an empty action...)
You can expect an empty action, but the index.php thing has been the behavior for 18 years (and wasn't done by me). In general you have to define option and task by hidden input in POST forms and GET forms will default to the current URL. Geniuses in the past have decided that it is uncool to have option as a hidden input, because you could then see that the site is made with Joomla (eww) and thus they requested this to be encoded in the form action. All praise be the SEOs.
Honestly, I'd love the whole system to be different, but we are all working in the confines of a system which was whacked together in Joomla 1.5 and since then has not been fundamentally changed. Mainly because every change is considered to be an impossible b/c break.
Long story short: Don't shoot the messenger.
I've just restored again the human test result in the issue tracker as all changes after that were only changes in the order of html attributes, which I've just reviewed with success, so there was no functional change since the test.
One more test needed.
I have tested this item ✅ successfully on 20782c8
Tested independently. Reverted the applied patch. Redirect does not work. Applied updated patch, redirect works. Thank you for the Fix!
I have tested this item ✅ successfully on 20782c8
Thanks you for Testing @coolcat-creations @dautrich @webnet-assmann
Thank you for Testing @coolcat-creations @dautrich @webnet-assmann
@LadySolveig Can it be that the system tests need some adjustment to this PR? Currently they are consistently failing for both database types at
Running: site/components/com_content/Article.cy.js (59 of 148)
Test in frontend that the content article form
1) can edit an article in menu item
2) "after each" hook for "can edit an article in menu item"
The problem is that the wrong success message is now being used.
Before this PR it was "Article Submitted" and thats what the test is looking for
With this PR it is now "Article Saved" - hence the failure
I have tested this item 🔴 unsuccessfully on 20782c8
See failing automated tests and explanation #45619 (comment)
Cypress test for the frontend edit would now unfortunately also have to be checked. My guess is that the test has already been written on the wrong behaviour. But I haven't had time to look at it yet.
May I ask you if you could validate this for me if you have time @muhme
exactly what I wrote yesterday.
The correct and intended message when submitting a new article is "Article Submitted" not "article saved" . Saved should be on edits not new article submissions,
For example on one of my sites I have a language override for "article submitted" which says "Thank you for your submission it will be reviewed by an editor before publishing". With the changes in this PR that language override is not used and its not appropriate to edit the "article saved" message
If ti helps at all the relevant code for the messages is at
joomla-cms/libraries/src/MVC/Controller/FormController.php
Lines 726 to 729 in 1594ea7
Thank you @brianteeman I have opened it now and have a look, that helps me a lot!
With the new url index.php&a_id
the line above gives null
as result
with the old one index.php&option=com_content&a_id
it gives 0
I think with this fix from you this will be stripped out right?
Revertet
Would fix it this way, but not sure if this could affect anything else. Can you also have a look? @Hackwar @bembelimen
This is the commit - 5607570
Would you be so kind as to test again whether it now works as expected? @brianteeman
At least the system tests are now running again.
But since it is the global FormController that I have now changed, maybe one or two tests in the backend would be good to validate that something has not got mixed up here.
I have tested and I have not noticed anything, but I would be happy about validation.
too soon sorry
Category | Front End com_config com_contact com_content com_users | ⇒ | Front End com_config com_contact com_content com_users Libraries |
Would fix it this way, but not sure if this could affect anything else. Can you also have a look? @Hackwar @benjaminpick
@bembelimen I guess you were meant.
The test now throws a php warning here because the key is not there.
So another problem with the striped id from the url.
Would you be so kind as to test again whether it now works as expected? @brianteeman
At least the system tests are now running again.
But since it is the global FormController that I have now changed, maybe one or two tests in the backend would be good to validate that something has not got mixed up here.
I have tested and I have not noticed anything, but I would be happy about validation.
Could you also have a look @bembelimen if my new fix could break anything I have overlooked? ec1d7bb
The issue I commented on regarding the string on new article submissions has now been resolved.
May I ask you if you could validate this for me if you have time @muhme
@LadySolveig Sorry, I didn't have time earlier. And I see you solved the problem with the System Tests yourself, which is the best solution anyway 👍
As the link to the downloads (and custom update URL) created by Drone for this PR are not easy to find now as we have these many additional CI checks, here is the link: https://artifacts.joomla.org/drone/joomla/joomla-cms/5.3-dev/45619/downloads/85715/
@webnet-assmann @coolcat-creations @dautrich Just if you have the time, could you test this PR again? I ping you all 3 so it's more likely to get 2 tests. So if you come to test and see here on GitHub that it has already 2 tests, you are done here, and can use the saved time for testing some other PR ;-)
Cancelled, see comment below this one.
Please do not perform any additional tests. I'm setting the PR on draft because it turned out that the additional fix now triggers the wrong behaviour again and the menu is not found and thus the redirects no longer work.
Category | Front End com_config com_contact com_content com_users Libraries | ⇒ | Front End com_config com_contact com_content com_users |
Category | Front End com_config com_contact com_content com_users | ⇒ | Front End com_config com_contact com_content com_users Libraries |
I don't see anywhere where I can enter a formal test but then again I'm not that familiar with Github.
See my test results as an image below, hopefully clear.
I also tested the redirect of a Login Menu item and it worked correctly, both the Menu Item Login Redirect and the Menu Item Logout Redirect.
I hope this will be followed up further and a solution will be quickly incorporated into a future Joomla version.
These incorrect redirects make creating and editing articles in the front-end very confusing, and I'm having a hard time explaining it to end users.
It's also unclear to me whether PR 45254 will now be deprecated and added to PR 45619; they are indeed the same problems.
I'd like to help further, but I'm not sure how?
Any news on this?
I have tested this item ✅ successfully on ba6f7a8
Tested successfully, thank you for the fix!
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/45619.