Problem/Motivation
Given a multilingual environment with path prefixes as the language determination method, as well as path prefixes added for every language (also for default/english), 2 problems arise:
- Going to admin/reports/status and clicking on the link "database update script" under database updates, results in a page not found (Drupal goes to /language_prefix/update.php
- Go to /update.php manually and click continue. This goes well. However, when proceeding and clicking on "Apply pending updates" a page not found is displayed again (URL at this point: "/language_prefix/update.php/run"
. Again, manually removing the language prefix results in the updates being executed properly
My site language settings:
When I try to update from RC3 to RC4...
[1] http://mysite/update.php - Overview page
[2] http://mysite/update.php/selection - Review updates page
[3] In this step I clicked the button for run the three or four updates and I get a 404 error:
http://mysite/es/update.php/run
To fix this, I had to edit the HTML code and remove language prefix ('/es' - My browser default language) from the Run updates button.
Drupal log (remember that I can finish my update editing the HTML code):
Attach a video:here
Proposed resolution
Apply the same URL generation for this link like others in the update process. Add tests.
Remaining tasks
None.
User interface changes
None.
API changes
None.
Data model changes
None.
Comments
Comment #2
facine CreditAttribution: facine commentedComment #3
dawehnerThis could be basically the same problem as #2583799: Install new module exposes bug in UnroutedUrlAssembler.buildLocalUrl() resulting in duplicated paths in URLs, such as /core/authorize.php/core/authorize.php but could be something else, so needs investigation.
Comment #4
facine CreditAttribution: facine commentedComment #5
facine CreditAttribution: facine commentedComment #6
cilefen CreditAttribution: cilefen commented@facine Are you using Apache or some other server like Nginx?
Comment #7
facine CreditAttribution: facine commented@cilefen: Apache/2.2.31
Comment #8
facine CreditAttribution: facine commentedComment #9
facine CreditAttribution: facine commentedComment #10
facine CreditAttribution: facine as a volunteer commentedThis patch works for me.
Comment #11
facine CreditAttribution: facine as a volunteer commentedComment #12
Gábor HojtsyI cannot tell why/how is this fixing the issue honestly. It is not explained in the issue summary either. Anyway, would be good to have tests for so we can ensure it does not regress.
Comment #13
facine CreditAttribution: facine as a volunteer commentedBecause this:
Returns "/es/update.php/run" and this page display a 404 error.
\Drual\Url returns a path with language prefix.
And my solution is the same way that has used in the selection step:
http://cgit.drupalcode.org/drupal/tree/core/modules/system/src/Controlle...
Comment #14
facine CreditAttribution: facine as a volunteer commentedComment #15
alvar0hurtad0Hello all,
I've reproduced the issue, and the patch works fine.
Where is the best place to code the test?
Comment #16
Gábor HojtsyThere is Drupal\system\Tests\Update\UpdatePathTestBase which is used in many tests to check for how the update.php process works. Either one of those should be extended or a new one added based on UpdatePathTestBase AFAIS.
Comment #17
Gábor HojtsyClosing #2607156: update.php does not work properly with language prefixes as duplicate.
Comment #18
agoradesign CreditAttribution: agoradesign commentedComing from #2607156: update.php does not work properly with language prefixes - this patch here from this thread is an elegant way to solve the problem :) But as far as I can see, this only solves 50% of the problem. I'm pretty sure, that the link on the status report page is still wrong, as I had to patch the DbUpdateController as well as the system_requirements() function.
Please see my patch from the other thread here: https://www.drupal.org/files/issues/2607156-4.patch
Comment #19
agoradesign CreditAttribution: agoradesign commentedCombined our two patches into one... only tests are missing now :)
PS: patch is so tiny, so I skip the interdiff...
Comment #20
facine CreditAttribution: facine as a volunteer commented@agoradesign: Oh, thanks!
Someone can try to explain who to make the tests? I've never made one...
Comment #21
cilefen CreditAttribution: cilefen commentedIn this case, you may be able to add something to Drupal\system\Tests\Update\UpdateScriptTest.
Comment #22
agoradesign CreditAttribution: agoradesign commentedAdded a test that simulates running an update on update.php in a multilingual environment, using a language-based path prefix
Comment #23
agoradesign CreditAttribution: agoradesign commentedComment #24
cilefen CreditAttribution: cilefen commented@agoradesign Can you upload a test-only patch along with the full patch so we can be sure it fails?
Comment #25
agoradesign CreditAttribution: agoradesign commented@cilefen: sure... I'm currently adding a few lines to the test case, inlcuding a new check, if the link to update.php on the status report page is correct (without language prefix)
Comment #26
agoradesign CreditAttribution: agoradesign commentedhere we go... updated the test + provided a test-only patch :-)
Comment #28
Gábor HojtsyLooks good to me. Once issue summary is updated, I can set it to RTBC :)
Comment #29
agoradesign CreditAttribution: agoradesign commentedLike this?
Comment #30
Gábor HojtsyTouched up the proposed resolutions a bit. Looks good now!
Comment #31
alexpottI'm pretty sure this is a critical bug since if we have a critical bug that requires an upgrade path multilingual sites will not be able to apply it.
It's great that this adds test coverage.
I'm a bit concerned about the routing assumptions this is making. It seems as though any use of the system.db_update route in a module will be dangerous because of this so should we able to add that to the routing information - opt out of path processing or something.
Comment #32
agoradesign CreditAttribution: agoradesign commented@alexpott: That's maybe worth a discussion, but this shouldn't be part of this issue/patch. As you can see, the use of the system.db_update route was not added by the patch, but is already part of the original code
Comment #33
alexpott@agoradesign - yes I know that the patch does not add the route - but what the bug reveals is that we need a way of disabling path processing for specific routes. We have a very similar problem on #2583799: Install new module exposes bug in UnroutedUrlAssembler.buildLocalUrl() resulting in duplicated paths in URLs, such as /core/authorize.php/core/authorize.php
@dawehner suggested in IRC that we should be able to do something like
$route->setOption('url_options' => ['path_processing' => FALSE]))
Comment #34
dawehnerJust to clarify this statement, we should add support for it.
Comment #35
dawehnerHere is a patch just for this general functionality.
Comment #36
jibranCan we add this test to the patch as well?
Comment #37
swentel CreditAttribution: swentel commentedMerged in that test
Comment #38
xjmThe D8 committers agreed that this is a critical issue.
Comment #40
swentel CreditAttribution: swentel commentedWoops
Comment #41
Gábor HojtsyNice trick! Also a backwards API addition, so should be possible in 8.0.x I hope :)
Comment #42
catchYes agreed that all looks fine for a patch release and nice fix.
Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!
Comment #46
Gábor HojtsyThanks all, yay!