Replace system.filter::protocols with container parameters

Just a migrate test failure. Easy fix? :)

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -72,19 +72,18 @@ class UrlGenerator implements UrlGeneratorInterface {
+   *   An array of protocols allowed for url generation.

+++ b/core/lib/Drupal/Core/Utility/UnroutedUrlAssembler.php
@@ -40,14 +40,13 @@ class UnroutedUrlAssembler implements UnroutedUrlAssemblerInterface {
+   *   An array of protocols allowed for url generation.

s/url/URL/

As mentioned, I think we should simply drop that from the migration. We did that as well for the private file path for example which is now in $settings.php

This looks great!

Just one last question before this is RTBC:

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -72,19 +72,18 @@ class UrlGenerator implements UrlGeneratorInterface {
+    UrlHelper::setAllowedProtocols($filter_protocols ?: ['http', 'https']);

+++ b/core/lib/Drupal/Core/Utility/UnroutedUrlAssembler.php
@@ -40,14 +40,13 @@ class UnroutedUrlAssembler implements UnroutedUrlAssemblerInterface {
+    UrlHelper::setAllowedProtocols($filter_protocols ?: ['http', 'https']);

Should we document why we need the fallback here?

I personally think it'd be great if #23 would be added as a comment to the patch, to explain the need for #22. But since @dawehner doesn't feel the same way AFAICT, let's see what a committer thinks. Because other than that, this is ready.

RTBC + 1, I think it is fine to have a safe fallback.

#26: What about:

During tests the ContainerParameter filter.protocols might not be available, in that case use a fallback.

However that is wrong, too as getParameter() throws an Exception when a parameter cannot be found, so either we write a wrapper that works like variable_get() / config() with default values or we catch the Exception ...

(See: https://github.com/symfony/DependencyInjection/blob/master/ParameterBag/...)

So currently the fallback should never be used anyway ...

Edit: X-post with #28

Discussed in IRC:

CNW as resolveValue() / getParameter() of ParameterBag clearly throws an Exception and that parameter should never be empty.

If we wanted a fallback, then we should add a generic Drupal::getParameter() function or a parameter service or such, that allows to specify a default value as fallback.

But then we are again way on our way to re-invent config, so probably not worth it ...

Thanks, back to RTBC

This issue is a major task that will improve performance significantly and the disruption it introduces is limited. Per https://www.drupal.org/core/beta-changes, this is a good change to complete during the Drupal 8 beta phase. Committed 0613685 and pushed to 8.0.x. Thanks!

+++ b/core/tests/Drupal/Tests/Core/Utility/UnroutedUrlAssemblerTest.php
diff --git a/sites/default/default.services.yml b/sites/default/default.services.yml
old mode 100644
new mode 100755

Fixed the mode change on commit.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -466,8 +466,8 @@ public function preHandle(Request $request) {
     // Set the allowed protocols once we have the config available.
-    $allowed_protocols = $this->container->get('config.factory')->get('system.filter')->get('protocols');
-    if (!isset($allowed_protocols)) {
+    $allowed_protocols = $this->container->getParameter('filter_protocols');
+    if (!$allowed_protocols) {
       // \Drupal\Component\Utility\UrlHelper::filterBadProtocol() is called by
       // the installer and update.php, in which case the configuration may not
       // exist (yet). Provide a minimal default set of allowed protocols for
       $allowed_protocols = array('http', 'https');
     }
     UrlHelper::setAllowedProtocols($allowed_protocols);

The code comment still talks about the *config* possibly not existing yet.
Is that "if" case still relevant ? (the parameter not existing in $this->container ?) Not sure about the installer, but probably not true for update.php now that it's a regular route & controller ?

#38 The parameter not existing would be an Exception, so the only case when it could be NULL or FALSE or '' is if its NULL or FALSE or '' in the container.

Not sure if that can happen during installation.

@FabianX is right, looks like that snippet had a straighforward conversion from "if (!config->get())" to "if (!container->getParameter())", but both sources don't behave the same on "doesn't exist" (and that "doesn't exist" doesn't occur on the same cases regarding install.php / update.php)

If I get the current situation right, It seems this "if (not exists)" is currently dead code, since the "not exists" case would throw an exception one line above, which doesn't seem to happen since we're currently green ?

#41: That is correct.

Posted a patch in #2541560: Followup for [#2529514] - remove dead code then

This removed a migration. I don't see anyone from the migration team weighing in here so should there be a follow up to make sure this is addressed or are we just not going to support upgrading them?