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

Fixed: Wrong url to update.php if language prefix in use #5092

Closed
indigoxela opened this issue May 16, 2021 · 18 comments · Fixed by backdrop/backdrop#3679
Closed

Fixed: Wrong url to update.php if language prefix in use #5092

indigoxela opened this issue May 16, 2021 · 18 comments · Fixed by backdrop/backdrop#3679

Comments

@indigoxela
Copy link
Member

indigoxela commented May 16, 2021

Description of the bug

On multilingual sites the first link on the status page to run update.php is wrong, as it includes the language prefix - it shouldn't.

Steps To Reproduce

On a site that uses language prefixes:

  1. Install a module update manually (on the file system)
  2. Go to admin/reports/status
  3. Click on the first link

status-page-right-and-wrong-url

Actual behavior

Land on a 404 page The requested page "/en/core/update.php" could not be found. (note the language prefix)

Expected behavior

The link should not contain the language prefix, as the page /core/update.php is a real file.

Additional information

Here's the part of code in core/modules/system/system.install:

        if ($current_schema < $latest_schema) {
          $requirements['update']['severity'] = REQUIREMENT_ERROR;
          $requirements['update']['value'] = $t('Out of date') . ' (' . l(t('Run database updates'), 'core/update.php'). ')';
          $requirements['update']['description'] = $t('Some modules have database schema updates to install. You should run the <a href="@update">database update script</a> immediately.', array('@update' => base_path() . 'core/update.php'));
          break;
        }

Two variants in use:

l(t('Run database updates'), 'core/update.php') // wrong
base_path() . 'core/update.php' // right

An additional problem with the description:

Some modules have database schema updates to install ... in 90% it's not a db update, but a config update. But that might make sense as an extra issue (wording usually needs more discussion and the term "database updates" is used all over the place).

@indigoxela
Copy link
Member Author

I just realized that I've been involved (testing), when this line of code has been touched last time - and I didn't check for multilingual. Shame on me. 😞 So let's really-really fix it this time.

@jayelless
Copy link

I am working on this issue. I can't seem to assign it to myself, as I don't have edit permission on this issue.

@jayelless
Copy link

I cant add labels either as described in the documentation :(

@jayelless
Copy link

Pull request generated.

@indigoxela
Copy link
Member Author

I can't seem to assign it to myself, as I don't have edit permission on this issue.

I see. I asked for help in our chat. Stay tuned.

Many thanks for the PR.

@indigoxela
Copy link
Member Author

I like that you re-use the same variable 👍 , but the substitution should be done slightly different. See my comment on your code for an explanation.

@olafgrabienski
Copy link

I can't seem to assign it to myself ...

I was able to assign you (can't change your permission, though).

@klonos
Copy link
Member

klonos commented Aug 23, 2021

Sorry for taking so long to review this small change. Code looks good @jayelless, and I also like the reuse of the variable in both places 👍🏼

@findlabnet since you reported facing this issue, do you have time to test this and mark it as "works for me" if everything works as expected? Thanks

@indigoxela, @olafgrabienski, or anyone else, mind setting the milestone to the next bug fix release? Thanks.

@findlabnet
Copy link

Tested.
The language prefix (in my case "en") has been removed from the URL, so the update is done in the default language, not English, and 404 is not returned.
Not perfect, but it works.

@indigoxela
Copy link
Member Author

The language prefix (in my case "en") has been removed from the URL

That's the purpose of this bugfix. Otherwise the link to the existing file upldate.php wouldn't work.

Not perfect, but it works.

Very encouraging. Maybe you want to file an alternate PR?

@findlabnet
Copy link

Don't get me wrong, please. The fix is done.
But switching me to a language that may not be so familiar to me makes me wonder if I may have done something wrong.

@indigoxela
Copy link
Member Author

But switching me to a language that may not be so familiar to me makes me wonder if I may have done something wrong.

I wouldn't know how to avoid it, that's why I asked you for an alternate PR. Or point us to a D7 patch that fixes it - honestly, I doubt there is any.

I assume, you think it "works for you", so I set the label accordingly. Otherwise set it back to "needs work" - but be aware that this could mean that you'll wait a lot longer for a solution.

@laryn
Copy link
Contributor

laryn commented Aug 23, 2021

But switching me to a language that may not be so familiar to me makes me wonder if I may have done something wrong.

Would it be worth displaying an informational message indicating you've been switched to the default language to run the update?

@klonos
Copy link
Member

klonos commented Aug 23, 2021

Or point us to a D7 patch that fixes it...

The respective issue in Drupal-land is https://www.drupal.org/project/drupal/issues/2616164 (with https://www.drupal.org/project/drupal/issues/2607156 closed as a duplicate), but it was raised and fixed only for D8 - not D7.

There is a test in the patch that was committed, which may be worth adding to Backdrop.

@klonos
Copy link
Member

klonos commented Aug 23, 2021

...the update is done in the default language, not English ... Not perfect, but it works.

I've searched to find any follow-up issue in drupal.org to make update.php work in multilingual situations, but I couldn't find any. It seems that our Drupal brethren may have accepted that as an architectural fact ("works as designed") 🤷🏼

@findlabnet
Copy link

At this time I cannot provide alternate PR.
So "works for me" is the same as "works as designed".
Thank you all for your participation!

@klonos
Copy link
Member

klonos commented Aug 23, 2021

I've raised #5171 as a separate task (but I suspect that we won't find a solution, unless we do something radical, which would be a 2.x thing).

I see no reason to hold this issue here further, but to add the test that went in for D8/D9: https://git.drupalcode.org/project/drupal/-/blob/9.2.x/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php#L589 (or do people prefer this to be a follow-up as well?)

@quicksketch
Copy link
Member

Thanks @jayelless, @indigoxela, @klonos, and @findlabnet! I merged backdrop/backdrop#3679 into 1.x and 1.19.x.

@jenlampton jenlampton changed the title Wrong url to update.php if language prefix in use Fixed: Wrong url to update.php if language prefix in use Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants