At the moment we persist the url_generator service. This service depends on many services that are not persisted and also has in bound and out bound path processor injected through a compiler pass. Basically it just does not make sense to persist this service.

The place where this is tricky is update.php because it futzes with it to present the correct url to home and admin pages.

The current approach is to use the UpdateServiceProvider to alter the url_generator service by adding the necessary method calls to create the correct URLs. There is no risk of compiling this container since the DrupalKernel is instantiated with dumping disabled in update_prepare_d8_bootstrap().

Persistence of the url_generator was added in https://drupal.org/comment/7212822#comment-7212822

CommentFileSizeAuthor
#9 2176669.9.patch5.45 KBalexpott
#9 7-9-interdiff.txt2.63 KBalexpott
#7 2176669.7.patch5.5 KBalexpott
#7 2-7-interdiff.txt811 bytesalexpott
#2 2176669.2.patch5.29 KBalexpott
d8.url_generator_persist.patch1.05 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, d8.url_generator_persist.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Needs review
FileSize
2176669.2.patch5.29 KB

Maybe this is a better way of ensuring that the url generator behaves as we want it to.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

FYI futzing the url generator for update.php was done in #2017769: Links from update_helpful_links() should point to site root and not to update.php

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Issue summary: View changes
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Persisting the url generator is certainly a bad workaround, so neither does symfony fullstack: https://github.com/symfony/FrameworkBundle/blob/master/Resources/config/...

  1. +++ b/core/includes/update.inc
@@ -108,6 +108,7 @@ function update_prepare_d8_bootstrap() {
+  $GLOBALS['conf']['UpdateServiceProviderOverrides'] = TRUE;

+++ b/core/lib/Drupal/Core/DependencyInjection/UpdateServiceProvider.php
@@ -18,33 +17,51 @@
+    if (!empty($GLOBALS['conf']['UpdateServiceProviderOverrides'])) {

    We could also replace that global variable by a kernel parameter. 

  2. +++ b/core/lib/Drupal/Core/DependencyInjection/UpdateServiceProvider.php
@@ -18,33 +17,51 @@
+    $definition->addMethodCall('setScriptPath', array(''));

    I wonder whether we should explain how setScriptPath('') will work.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
2-7-interdiff.txt811 bytes
2176669.7.patch5.5 KB

@dawehner not sure how we could actually implement your suggestion in to use kernel (or container) parameters.

Patch documents why we are using setScriptPath().

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented 
  1. +++ b/core/lib/Drupal/Core/DependencyInjection/UpdateServiceProvider.php
@@ -18,33 +17,54 @@
+  public function alterUrlGenerator(Request $request) {
+

    Is this empty method important? 

  2. +++ b/core/lib/Drupal/Core/DependencyInjection/UpdateServiceProvider.php
@@ -18,33 +17,54 @@
+    // determined by \\Drupal\Core\Routing\UrlGenerator::setRequest() is invalid

    two \\ (sorry!) 

  3. +++ b/core/lib/Drupal/Core/DependencyInjection/UpdateServiceProvider.php
@@ -18,33 +17,54 @@
+    // once /core has been removed from the base path.

    Maybe wrap in ''. 

  4. +++ b/core/lib/Drupal/Core/DependencyInjection/UpdateServiceProvider.php
@@ -18,33 +17,54 @@
+        ->register("cache_factory", 'Drupal\Core\Cache\MemoryBackendFactory');

    I know this is c&p but this is the only one using double quotes! Again, sorry. 

  5. +++ b/core/includes/update.inc
@@ -108,6 +108,7 @@ function update_prepare_d8_bootstrap() {
+  $GLOBALS['conf']['UpdateServiceProviderOverrides'] = TRUE;

+++ b/core/update.php
@@ -86,7 +86,7 @@ function update_helpful_links() {
+  $GLOBALS['conf']['UpdateServiceProviderOverrides'] = FALSE;

    I feel that this variable should just use underscores, and not camel casing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
7-9-interdiff.txt2.63 KB
2176669.9.patch5.45 KB

1. Fixed. Not at all - it was part of another approach I was exploring :)
2. Fixed
3. Fixed
4. Sure
5. Sure

dawehner’s picture

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

This is ready now, after the review of damian.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Looks fine.

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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