As part of all of the changes to the routing system we've made global $script_path obsolete. The only usages are in tests yet we assign it in every request.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because the global $script_path should not be used.
Prioritized changes This is a followup of all the work on the url() and l().
Disruption Contrib will have to change it is using $script_path but it should not be and by fixing this code becomes more testable.
CommentFileSizeAuthor
#13 interdiff.txt866 bytesdawehner
#13 2426495-13.patch5.27 KBdawehner
#7 2426495-2.7.patch5.27 KBalexpott
#4 interdiff.txt1.01 KBdawehner
#4 2426495-4.patch5.25 KBdawehner
#2 2426495-2.patch4.25 KBdawehner
d8.script-path.patch6.54 KBalexpott

Comments

dawehner’s picture

dawehner
Primary language German
commented

Just an idea.

Instead of having the same kind of code twice, what about providing a method in our own variant of the routing context?

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 2426495-2.patch4.25 KB

Let's better replace all the usages of $script_path

Status: Needs review » Needs work

The last submitted patch, 2: 2426495-2.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new 2426495-4.patch5.25 KB
new interdiff.txt1.01 KB

Alright, one less problem.

Crell’s picture

Crell commented

#4seems good to me, and the diffstats go the right direction. :-)

fabianx’s picture

fabianx commented
Status: Needs review » Reviewed & tested by the community

RTBC, looks great to me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2426495-2.7.patch5.27 KB

Rerolled because #2426489: Remove request_uri() landed and we still need a CR.

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: -Needs change record

Added one. Do you think this is back to RTBC?

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Would be awesome to mention in which cases specifically the first example doesn't work and what the specific difference between the two is.

dawehner’s picture

dawehner
Primary language German
commented

I just realized that $script_path is something which just exists for 8.0.x ... so do we really think we need a change record?

Crell’s picture

Crell commented
Status: Needs review » Reviewed & tested by the community

And back.

I don't think $script_path was ever part of an API in D8, so if it didn't exist prior then there's no need for a CR.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs review

This seems fine, other than:

+++ b/core/modules/system/src/Tests/Path/UrlAlterFunctionalTest.php
@@ -87,8 +88,7 @@ function testUrlAlter() {
+    $final = Url::fromUserInput('/' . $final)->toString();

This is not user input. Should we replace with an explicit internal:// instead? Or do we not care, since this is "just" a test?

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 2426495-13.patch5.27 KB
new interdiff.txt866 bytes

I think think internal:// is the WORST thing we ever came up with, anyway, let's use it.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

I know, sorry. :(

Committed and pushed to 8.0.x. Thanks!

  • webchick committed cbcf272 on 
    Issue #2426495 by dawehner, alexpott: Remove the global $script_path

Status: Fixed » Closed (fixed)

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