Afaik we need to use the global $base_url to fix this :(

From code : core/includes/bootstrap.inc

// @todo Temporary BC for install.php, update.php, and other scripts.
// - #1547184: Refactor Core scripts
// - #1546082: Follow-up to variable_get('clean_url') removal

I also checked #1648004: Helpful links use base_path() when they don't have to for more clues how to solve this.

Files: 
CommentFileSizeAuthor
#40 drupal-2017769-40.patch2.54 KBpplantinga
PASSED: [[SimpleTest]]: [MySQL] 57,187 pass(es).
[ View ]
#38 drupal-2017769-38.patch0 bytespplantinga
PASSED: [[SimpleTest]]: [MySQL] 57,106 pass(es).
[ View ]
#37 base-url-2017769-37.patch3.02 KBlongwave
PASSED: [[SimpleTest]]: [MySQL] 57,186 pass(es).
[ View ]
#26 base-url-2017769-26.patch1.17 KBpplantinga
PASSED: [[SimpleTest]]: [MySQL] 56,663 pass(es).
[ View ]
#17 drupal-2017769-17.patch2.54 KBkatbailey
PASSED: [[SimpleTest]]: [MySQL] 58,275 pass(es).
[ View ]
#2 drupal-2017769-2.patch2.51 KBelachlan
PASSED: [[SimpleTest]]: [MySQL] 58,036 pass(es).
[ View ]
#1 core-update-result-links-2017769-1.patch2.51 KBclemens.tolboom
PASSED: [[SimpleTest]]: [MySQL] 57,334 pass(es).
[ View ]

Comments

clemens.tolboom’s picture

Status:Active» Needs review
StatusFileSize
new2.51 KB
PASSED: [[SimpleTest]]: [MySQL] 57,334 pass(es).
[ View ]
elachlan’s picture

StatusFileSize
new2.51 KB
PASSED: [[SimpleTest]]: [MySQL] 58,036 pass(es).
[ View ]

Re rolled the patch. This needs some attention. This should have been committed ages ago.

elachlan’s picture

Component:base system» database update system

Change to be appropriate.

clemens.tolboom’s picture

Component:database update system» install system

@elachlan afaik there is no difference between #3 and #2

Hmmm ... I got this problem when installing. Did you got this while updating?

elachlan’s picture

Issue tags:+d8ux

Yes.

If you run Update.php then the links will link back to update.php.

Hence why I tagged it as database update system.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Perfect, that is the design of url(), but shouldn't we use $base_path . '' ... i guess this is not possible?

elachlan’s picture

$base_path returns "/" pretty much.

I think this is for the best. We now have tests to test for regression as well.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
+++ b/core/update.phpundefined
@@ -166,14 +166,15 @@ function update_script_selection_form($form, &$form_state) {
+  global $base_url;

I think you might be abe to use Drupal::request()->getBaseUrl(); here instead of the global...

alexpott’s picture

Marked #2020141: Wrong link to admin pages after succesful update as a duplicate... jeroen12345, suhel.rangnekar, marcoscano should be added to the commit message to ensure there work on producing exactly the same patch :) is credited.

elachlan’s picture

Status:Needs work» Reviewed & tested by the community

Marking as RTBC.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

Sorry... this isn't rtbc... we need to address the issue raised in #8

elachlan’s picture

In collaboration with alexpott, we worked out that Drupal::request()->getBaseUrl() isn't working how we would like.

So for now it is best to use global $base_url;

This is due to Url() https://api.drupal.org/api/drupal/core!includes!common.inc/function/url/8

At the moment it isn't working properly, this comment is left in the code in common.inc

// Fallback to using globals.
// @todo Remove this once there is no code that calls url() when there is
//   no request.

alexpott will check on outstanding issues tomorrow with a few others.

alexpott’s picture

+++ b/core/update.phpundefined
@@ -166,14 +166,15 @@ function update_script_selection_form($form, &$form_state) {
function update_helpful_links() {
+  global $base_url;
   $links['front'] = array(
     'title' => t('Front page'),
-    'href' => '<front>',
+    'href' => $base_url,
   );
   if (user_access('access administration pages')) {
     $links['admin-pages'] = array(
       'title' => t('Administration pages'),
-      'href' => 'admin',
+      'href' => $base_url . '/admin',
     );
   }
   return $links;

I think what we can do this...

function update_helpful_links() {
   $links['front'] = array(
     'title' => t('Front page'),
     'href' => '<front>',
     'absolute' => TRUE,
   );
   if (user_access('access administration pages')) {
     $links['admin-pages'] = array(
       'title' => t('Administration pages'),
       'href' => 'admin',
       'absolute' => TRUE,
     );
   }
   return $links;
elachlan’s picture

That didn't work for me when I tried it.

alexpott’s picture

Yep me neither... what is very interesting is my I access update.php the baseUrl property on the url_generator is set to http://my.drupal8.site/core when the actual base url is http://my.drupal8.site

I think we're dealing with a deeper issue in the routing system / url generation system.

clemens.tolboom’s picture

Status:Needs work» Reviewed & tested by the community

Have you read the issue summary? $base_url is necessary. It has todo's in common.inc which should be addressed somewhere else.

Note the request to update.php has its url() pointing to update.php as the request came from it.

This patch is RBTC-ed by dawehner on my request so afaik it's good to go.

@alexpott I agree this is not perfect but we cannot patch url() as /update.php is the active request and not /index.php

katbailey’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new2.54 KB
PASSED: [[SimpleTest]]: [MySQL] 58,275 pass(es).
[ View ]

Here's a fix that uses the same technique as we use in the installer - setting the script path and base path to use on the generator.

elachlan’s picture

Works for me. Has my +1.

Status:Needs review» Needs work
Issue tags:-d8ux

The last submitted patch, drupal-2017769-17.patch, failed testing.

elachlan’s picture

Status:Needs work» Needs review
Issue tags:+d8ux

#17: drupal-2017769-17.patch queued for re-testing.

clemens.tolboom’s picture

Some notes:

- Patch looks great but does it work for the url's used in update php when results from either drupal_requirements_url() and drupal_current_script_url() are put through check_url()? Are all URLs ok which patch #17?

- I've tested the patch by changing user update 8018 to 8017 as it's code seems non destructive. The links on http://drupal.d8/core/update.php?op=results are ok.

pplantinga’s picture

The issue here is deeper than just update.php

In UrlGenerator::generateFromPath we refer to $this->basePath which is set to be Symfony's $request->getBasePath() which returns the currently executing script's location. It looks like we're assuming that index.php will be the executing script.

So anytime we're not in index.php (e.g. install.php, update.php, authorize.php, etc.) this will return a different value.

elachlan’s picture

Is it related to this issue? #2031353: URLgenerator broken for Drupal installed in a subdirectory - doesn't have a way to get a Drupal path

On that note, has anyone tested without drupal being installed in a subdirectory?

longwave’s picture

Marked #2029541: Links in update.php page don't point to the right location as duplicate, which has a similar patch to #17.

elachlan’s picture

#22 - could you explain what you mean by "It looks like we're assuming that index.php will be the executing script."

I couldn't find mention of it in UrlGenerator::generateFromPath.

pplantinga’s picture

StatusFileSize
new1.17 KB
PASSED: [[SimpleTest]]: [MySQL] 56,663 pass(es).
[ View ]

Sure, I'll elaborate.

In drupal 7 url() (the place where this code used to reside) the line reads

<?php
$base
= $options['absolute'] ? $options['base_url'] . '/' : base_path();
?>

Where base_path() is the global $base_path variable, which is the installation directory of drupal.

Now the line reads:

<?php
$base
= $options['absolute'] ? $options['base_url'] : $this->basePath;
?>

Where $this->basePath is set to be the directory of the currently executing script.

Perhaps this was done before scripts were moved to core/ and so the author didn't realize that $this->basePath would be different in different scripts.

I think probably the best solution is to make $this->basePath be based on the global $base_path variable again, and then change $this->scriptPath to include /core/ or whatever. Also, the comment on 'script' is misleading:

- 'script': Added to the URL between the base path and the path prefix. Defaults to empty string when clean URLs are in effect, and to 'index.php/' when they are not.

I think ideally, 'script' would just be a boolean indicating whether or not to include the script path since the path can be set using $this->setScriptPath()

Here's a patch just implementing the basePath change, and "fixing" the broken links on the update page. I'm kinda thinking more needs to be done, but I'm not sure if "API freeze" prevents this...

Status:Needs review» Needs work
Issue tags:-d8ux

The last submitted patch, base-url-2017769-26.patch, failed testing.

pplantinga’s picture

Status:Needs work» Needs review

#26: base-url-2017769-26.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, base-url-2017769-26.patch, failed testing.

pplantinga’s picture

Status:Needs work» Needs review
Issue tags:+d8ux

#26: base-url-2017769-26.patch queued for re-testing.

katbailey’s picture

The "assumption" here, if there is one, is that we are using a front controller - update.php and install.php are not proper front controllers, and so they need to be special-cased. We certainly do not want to be putting calls to base_path() in the generator. The solution in #17 is what we need here: tell the generator what script path and base path to use when generating urls in update.php or install.php because it cannot get them from the current path as it is a stupid non-front-controller path.

longwave’s picture

+$generator->setBasePath(str_replace('/core', '', $request->getBasePath()) . '/');

What if there is another directory beginning with "core" in the path?

pplantinga’s picture

Status:Needs review» Reviewed & tested by the community

Fair enough. This gets my +1 too then.

As for #32, this is a special case, so we need not worry about other cases.

YesCT’s picture

Issue tags:+RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

clemens.tolboom’s picture

Status:Reviewed & tested by the community» Needs work

Where are the tests done by #2#3 #17?

longwave’s picture

Marked #2044371: wrong links under update.php as duplicate.

@clemens.tolboom: #17 adds tests to ensure the links no longer contain "update.php"; if this is not enough, what else should be tested?

longwave’s picture

Status:Needs work» Needs review
StatusFileSize
new3.02 KB
PASSED: [[SimpleTest]]: [MySQL] 57,186 pass(es).
[ View ]

Oh, I see what you mean now :)

Attached is #26 plus tests from #17.

pplantinga’s picture

StatusFileSize
new0 bytes
PASSED: [[SimpleTest]]: [MySQL] 57,106 pass(es).
[ View ]

When I marked this as rtbc, I was referring to #17, not #26, sorry for the confusion. I'll attach it again just to make it clearer.

elachlan’s picture

#38 - You posted an empty patch.

pplantinga’s picture

StatusFileSize
new2.54 KB
PASSED: [[SimpleTest]]: [MySQL] 57,187 pass(es).
[ View ]

Someone needs a brain transplant (hint: the answer is me). here's an actual patch.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Nice

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Confirmed the additions to the tests fail without the fix...

Committed 79347b8 and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.