Followup for [#2529514] - remove dead code [#2541560]

See discussion in #2529514-38: Replace system.filter::protocols with container parameters and below

The patch that got committed did :

+++ 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.
- As @Fabianx points, the parameter not existing in the container would throw an Exception, so the only case when the if() code runs is if 'filter_protocols' exists and is an explicit NULL or FALSE or '' in the container, which seems unlikely (and not the kind of things we babysit ?)

It 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"
- the "doesn't exist" case actually seems to be dead code, since an exception would be thrown one line above, which doesn't seem to happen since we're currently green.

Thus, looks like we can simplifying this ? Patch coming up

Comment	File	Size	Author
#9	interdiff.txt	551 bytes	yched
#9	2541560_filter_protocols_simplify-9.patch	1.91 KB	yched
#1	2541560_filter_protocols_simplify-1.patch	1.83 KB	yched

Comments

Comment #1

yched commented 28 July 2015 at 19:48

Status:

Active

» Needs review

Status	File	Size
new	2541560_filter_protocols_simplify-1.patch	1.83 KB

Patch.

Comment #2

yched commented 28 July 2015 at 19:49

Parent issue:

» #2529514: Replace system.filter::protocols with container parameters

Comment #3

fabianx commented 28 July 2015 at 19:50

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -466,15 +466,7 @@ public function preHandle(Request $request) {
     // Set the allowed protocols once we have the config available.

That comment needs work, too.

+++ b/core/modules/filter/filter.module
@@ -501,7 +501,7 @@ function _filter_url($text, $filter) {
-  $protocols = \Drupal::getContainer()->getParameter('filter_protocols') ?: ['http', 'https'];
+  $protocols = \Drupal::getContainer()->getParameter('filter_protocols');

dawehner had one failing test here, lets see if this still passes without.

Comment #4

yched commented 28 July 2015 at 19:51

Issue summary:

View changes

Comment #5

dawehner

German

commented 28 July 2015 at 21:13

This issue feels a bit dangerous now, given that we might need to rebuild the container in order to be able to generate URLs again ...

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -466,15 +466,7 @@ public function preHandle(Request $request) {
-    UrlHelper::setAllowedProtocols($allowed_protocols);
+    UrlHelper::setAllowedProtocols($this->container->getParameter('filter_protocols'));

+++ b/core/modules/filter/filter.module
@@ -501,7 +501,7 @@ function _filter_url($text, $filter) {
-  $protocols = \Drupal::getContainer()->getParameter('filter_protocols') ?: ['http', 'https'];
+  $protocols = \Drupal::getContainer()->getParameter('filter_protocols');
   $protocols = implode(':(?://)?|', $protocols) . ':(?://)?';

I'm actually understand now why it didn't passed for me for a while ... this was the case because core.services.yml was not patched at some point.

Comment #6

yched commented 28 July 2015 at 22:30

@dawehner : not sure I follow exactly, but do you think we should keep some safeguard code in DrupalKernel::preHandle() ?

Like :

$filter_protocols = $this->container->hasParameter('filter_protocols') ? $this->container->getParameter('filter_protocols') : ['http', 'https'];
UrlHelper::setAllowedProtocols($filter_protocols);

I mean, I guess we could, but I'm not sure why we'd do this for 'filter_protocols' specifically, as opposed to all the other parameters / services that could go missing in a somehow incomplete container ? Especially if we don't seem to have a specific case of how such an incomplete container might happen (doesn't seem to be the case on installer ?)

Comment #7

yched commented 28 July 2015 at 22:32

As a side note, that's way outside the scope of that followup, but setting those "allowed protocols" in a global static in UrlHelper feels... very D7 ? :-)

Comment #8

fabianx commented 28 July 2015 at 23:27

Status:

Needs review

» Needs work

CNW for the comment change - that refers to config objects still.

Patch itself looks good.

Comment #9

yched commented 29 July 2015 at 12:08

Status:

Needs work

» Needs review

Status	File	Size
new	2541560_filter_protocols_simplify-9.patch	1.91 KB
new	interdiff.txt	551 bytes

Comment fixed.

Comment #10

fabianx commented 29 July 2015 at 12:19

Status:

Needs review

» Reviewed & tested by the community

Thanks, RTBC.

Comment #11

dawehner

German

commented 29 July 2015 at 12:24

As a side note, that's way outside the scope of that followup, but setting those "allowed protocols" in a global static in UrlHelper feels... very D7 ? :-)

I totally agree it does but do you have a better idea without changing these APIs? The url generator is not the first place where we might need it.

Comment #12

yched commented 29 July 2015 at 12:42

I totally agree it does but do you have a better idea without changing these APIs?

Not really ;-) Looks like cleaning this would mean refactoring UrlHelper::stripDangerousProtocols() (and the other UrlHelper methods that rely on it : isExternal(), filterBadProtocol()) from static methods to a service that gets the protocols injected, can hardly be called non-intrusive.

I guess this stands out more now that the protocols are in the container, ready for injection, whereas it was more "meh, procedural legacy in a low-level API" when they were still in config. I guess we can live with it :-)

Comment #13

dawehner

German

commented 29 July 2015 at 19:07

Yeah not sure where else beside Url generation you would actually use it though.

Comment #14

yched commented 29 July 2015 at 23:26

@dawehner : not sure I understand what you're hinting - you mean the existing UrlGenerator might be that service where stripDangerousProtocols() could live ?

Comment #15

dawehner

German

commented 30 July 2015 at 05:29

Well yeah I was wondering, which places do actually need this functionality, just by looking at core, it is certainly not a lot.

Comment #16

alexpott

he/they

English

🇪🇺🌍

commented 6 August 2015 at 11:07

Status:

Reviewed & tested by the community

» Fixed

Dead code is permitted to be removed in the beta evaluation. Committed e50271a and pushed to 8.0.x. Thanks!

Comment #17

6 August 2015 at 11:07

alexpott committed e50271a on 8.0.x

Issue #2541560 by yched, dawehner, Fabianx: Followup for [#2529514] -...

Comment #18

20 August 2015 at 11:14

Status:

Fixed

» Closed (fixed)

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

Followup for [#2529514] - remove dead code

Comments

Comment #1

Comment #2

Comment #3

Comment #4

Comment #5

Comment #6

Comment #7

Comment #8

Comment #9

Comment #10

Comment #11

Comment #12

Comment #13

Comment #14

Comment #15

Comment #16

Comment #17

Comment #18

Parent issue

News items

Our community

Documentation

Drupal code base

Governance of community