Create a Current Path Condition

Any progress here?

+++ b/core/modules/system/lib/Drupal/system/Tests/Condition/RequestPathConditionTest.phpundefined
@@ -0,0 +1,81 @@
+    $pages = "my/pass/page\r\nmy/pass/page2";

Do we need to check my/pass/* path?

Thanks you @EclipseGc. Patch looks fine to me. Everything looks great.
After doing little poking around in the core I finally understand

+++ b/core/modules/system/lib/Drupal/system/Plugin/Core/Condition/RequestPath.phpundefined
@@ -0,0 +1,84 @@
+    $request = $this->getContextValue('request');
+    $alias_manager = $this->getContextValue('alias_manager');

I going to RTBC a "Stalking Crell" issue :).

Not so fast ;-)

EclipseGC mentioned this to me this morning and I said I'd have a look tonight - I already see some problems with it (it shouldn't be special-casing the alias manager as this is only one example of something that messes with the incoming path - it should use the path processor manager instead) which I will elaborate on tonight.

Sorry for the vague comment earlier and unfortunately I still haven't had much time to spend on this (had hoped to throw together a test to show the problem I'm referring to) but my main concern is this: since #1910318: Make path resolution the responsibility of a series of PathProcessor classes, rather than one PathSubscriber class, the alias manager is no longer singled out as the one thing that alters the inbound path. Rather, we have a series of inbound path processors that act in sequence on the request path to resolve it to the system path. The alias manager is just one of the processors in this chain. You could have an arbitrary path processor that specifies that the path 'foo/bar' should resolve to 'some/path/alias' which in turn resolves to 'node/1'. If the path condition specified for a particular block was that it should show on 'foo/bar', then this would not work with the current setup as it would only check the condition against the system path and the path alias.

So what I'm thinking is that this plugin should get the path processor manager injected into it instead of the alias manager. But we'd need to make some changes so that the path processor keeps track of the various path resolutions along the way and exposes a getInboundPathResolutions() method or something, and then the condition would be checked against each one. Here is the PathProcessorManager class: http://drupalcode.org/project/drupal.git/blob/ea909a162c195e50271685a09c...

The drupal_match_path() logic should either be moved into the condition plugin if all other uses of it in core will be converted to uses of this plugin, or else it should be a method on the path processor manager, which is a service so we could keep drupal_match_path as a wrapper function around a call to Drupal::service('path_processor_manager')->matchPath() for other procedural code that still needs it.

That then just leaves drupal_strotolower() as the only other procedural function being called here which I'm not sure what to do about but it would be nice to get rid of it.

Anyway, I hope this makes sense and that I am not over-thinking it. It would be good to get Crell's thoughts on this too.

+++ b/core/modules/system/lib/Drupal/system/Plugin/Core/Condition/RequestPath.php
@@ -0,0 +1,84 @@
+ *     "request" = {
+ *       "class" = "Symfony\Component\HttpFoundation\Request"
+ *     },

Similar to my question in #1921540-42: Create a User Role Condition, how will this integrate with block caching? When this condition is used to affect what's in a block, then we'll want to cache the block by the request path, but not by, say, the client IP address, which is also part of $request. However, if we're saying the condition requires an entire $request object, then how does the SCOTCH controller know that path will be used (and therefore needs to factor into caching), but IP address won't be? Related: #1965990: Figure out how to integrate hooks, $request, and caching.

tagging

@Crell are you able to weigh in on #13? EclipseGc is awaiting confirmation before moving down that path.

+++ b/core/modules/system/lib/Drupal/system/Plugin/Core/Condition/RequestPath.php
@@ -0,0 +1,84 @@
+    $pages = drupal_strtolower($this->configuration['pages']);

This can now be replaced with Unicode::strtolower(), which is slightly less fugly.

+++ b/core/modules/system/lib/Drupal/system/Plugin/Core/Condition/RequestPath.php
@@ -0,0 +1,84 @@
+    $path_alias = drupal_strtolower($alias_manager->getPathAlias($path));

Ibid.

+++ b/core/modules/system/lib/Drupal/system/Plugin/Core/Condition/RequestPath.php
@@ -0,0 +1,84 @@
+    return drupal_match_path($path_alias, $pages) || (($path != $path_alias) && drupal_match_path($path, $pages));

This may be scope creep, but is there any way to turn drupal_match_path() into a service we can just inject here? It would be such a nice little service.

In fact, that would likely help resolve Kat's concern about aliases vs. path processors. That becomes that service's problem, and it can take the path processor manager as a dependency.

My other concern is passing in the request object directly, because I'm always nervous when passing the request object to something. Is there no way to pass in just a path, which then gets passed over to the service above and it takes care of mangling it as needed?

I haven't quite got this working. I haven't ventured into plugins and annotions land yet, so there might be something obvious someone else can help with?

Still need to convert everything else to use a PathMatcher service.

+++ b/core/lib/Drupal/Core/Path/PathMatcher.php
@@ -0,0 +1,36 @@
+    $regexps = &drupal_static(__FUNCTION__);

drupal_static() should never be used inside a class. Just save things to an object property.

+++ b/core/lib/Drupal/Core/Path/PathMatcher.php
@@ -0,0 +1,36 @@
+        '\1' . preg_quote(config('system.site')->get('page.front'), '/') . '\2',

This should be injected.

Added a factory and injecting the PathMatcher into the RequestPath rather than trying to use getContextValue(). Also made changes as per #20.

#22: 1921544-current-path-condition-22.patch queued for re-testing.

The plugin (request_path) did not specify an instance class. in Drupal\Component\Plugin\Factory\DefaultFactory::getPluginClass() (line 62 of /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Component/Plugin/Factory/DefaultFactory.php)

Not sure what I have got wrong here?

Talked with @dawehner at Drupalcon who told me about ContainerFactory, so we are using that to allow dependencies to be injected.

Also, need to Change ContainerFactory to check for existence of create method, as it's not always there.

Need to use correct method signature for create and ensure that the parent constructor is called correctly.

+++ b/core/modules/system/lib/Drupal/system/Tests/Condition/RequestPathConditionTest.phpundefined
@@ -0,0 +1,84 @@
+class RequestPathConditionTest extends DrupalUnitTestBase {

I nearly that you could write a php unit test, but then something called t() :(

Instead of using reflection, can we have a ContainerPluginInterface with create() on it?

Yes we should, and use class_implements('ContainerPluginInterface') instead. Can we do this in a follow up?

K

Fixed typo contruct => construct as well as some clean ups.

Post here to get some help from @dawehner at the sprint.

You had the plugin in the wrong directory && mixed up the context ...

#35: durpal-1921544-35.patch queued for re-testing.

Thanks @dawehner!!

+++ b/core/lib/Drupal/Core/Path/PathMatcher.phpundefined
@@ -0,0 +1,63 @@
+  /**
+   * The config factory.
+   *
+   * @var \Drupal\Core\Config\ConfigFactory
+   */
...
+  /**
+   * Creates a new PathMatcher
+   *
+   * @param ConfigFactory $config_factory
+   *   The config factory.
+   */
+  function __construct(ConfigFactory $config_factory) {
+    $this->configFactory = $config_factory;
...
+        '\1' . preg_quote($this->configFactory->get('system.site')->get('page.front'), '/') . '\2',

What about just storing the frontpage string in the pathMatcher but inject the ConfigFactory?

Only store the default front page as per #39.

We need to fix up the tests to use injection. We aren't using the alias_manager from the plugin context anymore.

I stuffed up and #40 didn't include #35 :-(

This does, and removes the setContext from the test as its now injected. :-)

Then please

+++ b/core/modules/system/lib/Drupal/system/Plugin/Condition/RequestPath.phpundefined
@@ -0,0 +1,139 @@
+ *    "alias_manager" = {
+ *       "class" = "Drupal\Core\Path\AliasManagerInterface"
+ *     }

Now we can remove that again.

Removes unused annotation in #43.

+++ b/core/lib/Drupal/Core/Path/PathMatcherInterface.phpundefined
@@ -0,0 +1,24 @@
+   * @return Boolean
+   *   value: TRUE if the path matches a pattern, FALSE otherwise.

Yeah the standard is just a little bit different.

Talked with @katbaily and @Crell about this issue at Drupalcon Portland, and agreed that we would keep PathMatcher as a service.

Also talked with @eclipsegc and agreed that we would not use ContainerFactory, and instead pass the alias_manager, and path_matcher services in via context.

I'll work on this while waiting in the LAX transit lounge and hopefully have a patch today.

This reverts all the use of ContainerFactory and dependency injection, and instead used context to get the needed dependencies for alias_manager and path_matcher as per #46.

It still uses the PathMatcher as this was agreed to be the best current solution.

(BTW airport wifi sucks).

Re-rolled and marked drupal_match_path as deprecated.

This looks fine to me for now as we agreed to punt on the bigger question of whether to stop special-casing the alias manager for things like this and use the path processor manager instead.
I made an attempt to convert the test to a unit test before realizing this is completely impossible until #1813762: Introduce unified interfaces, use dependency injection for interface translation and #1903346: Establish a new DefaultPluginManager to encapsulate best practices have landed. And actually, dawehner pointed this out already in #31 :-P

+++ b/core/modules/system/lib/Drupal/system/Plugin/Condition/RequestPath.php
@@ -0,0 +1,94 @@
+    $pathMatcher = $this->getContextValue('path_matcher');

The path manager context? What should a user configure to go in there?

No, dependencies should stay what they are: a dependency. Contextual data that a conditions needs to work is something different. So this should use regular dependency injection, e.g. via the static createInstance() method as for other plugins.

Hi @fago,

@eclipsegc, @crell, @katnbailey and had a discussion on this at the sprint, and believe we got closer to an understanding of the difference between plugins that are re-used in different 'contexts' (e.g. conditions) and those that are not (e.g. in views). The former should not be dependency injected, but the latter can be.

I think @eclipsegc probably needs to chime in here and make the same point he made to us.

Yes, but we've the problem already solved with #1863816: Allow plugins to have services injected - in a different way. So why do we need another solution based upon contexts for conditions/actions/..?

The problem with mixing it into the context is that there is no logic any more that connects the context with the service in a general way. E.g. how we'd Rules figure out which service to connect to the context and how would it figure out which contexts would have to be hidden from the UI because they are just dependencies in reality?

I'd have had the conversation in Portland if I had realized we weren't yet on the same page here. Apologies.

Yep, a pity we missed that. I've not realized that either :/

I'm with fago on this one, I had no idea we would be using context for DI. That makes absolutely no sense to me.

I came here after seeing this kind of thing in action in #2003482: Convert hook_search_info to plugin system.

WIth fago and tim.plunkett. This is insanity. It's bad enough we can't have contexts injected via arguments, let's please not try to pass dependencies into the same system.

Also, the "slave database" needs to stop being used as the sole use case for doing crazy things.

Hardcoding the class names directly is overkill, bypasses the DIC completely, is hard to read, and is a ton more work.

In addition, you now have to cope with being coupled to the container. From #2003482: Convert hook_search_info to plugin system:

/**
 * Executes a keyword search aginst the search index.
 *
 * @SearchExecutePlugin(
 *   id = "node_search_execute",
 *   title = "Content",
 *   path = "node",
 *   module = "node",
 *   context = {
 *     "plugin.manager.entity" = {
 *       "class" = "\Drupal\Core\Entity\EntityManager"
 *     },
 *     "database" = {
 *       "class" = "\Drupal\Core\Database\Connection"
 *     },
 *     "module_handler" = {
 *       "class" = "\Drupal\Core\Extension\ModuleHandlerInterface"
 *     }
 *   }
 * )
 */
class NodeSearchExecute extends ContextAwarePluginBase implements SearchExecuteInterface {
  static public function create(ContainerInterface $container, array $configuration, $plugin_id, array $plugin_definition) {
    if (empty($configuration['context'])) {
      $configuration['context'] = array();
    }
    if (empty($configuration['context']['plugin.manager.entity'])) {
      $configuration['context']['plugin.manager.entity'] = $container->get('plugin.manager.entity');
    }
    if (empty($configuration['context']['database'])) {
      $configuration['context']['database'] = $container->get('database');
    }
    if (empty($configuration['context']['module_handler'])) {
      $configuration['context']['module_handler'] = $container->get('module_handler');
    }
    return new static($configuration, $plugin_id, $plugin_definition);
  }

Following how the rest of all Dependency Injected code in core works, that would be:

use Drupal\Core\Entity\EntityManager;
use Drupal\Core\Database\Connection;
use Drupal\Core\Extension\ModuleHandlerInterface;

/**
 * Executes a keyword search aginst the search index.
 *
 * @SearchExecutePlugin(
 *   id = "node_search_execute",
 *   title = "Content",
 *   path = "node",
 *   module = "node"
 * )
 */
class NodeSearchExecute extends ContainerFactoryPluginBase implements SearchExecuteInterface {
  protected $entityManager;
  protected $database;
  protected $moduleHandler;
  
  public function __construct(array $configuration, $plugin_id, array $plugin_definition, EntityManager $entityManager, Connection $database, ModuleHandlerInterface $module_handler) {
    parent::__construct($configuration, $plugin_id, $plugin_definition);
    
    $this->entityManager = $entity_manager;
    $ths->database = $database;
    $this->moduleHandler = $module_handler;
  }
  
  static public function create(ContainerInterface $container, array $configuration, $plugin_id, array $plugin_definition) {
    return new static(
      $configuration, $plugin_id, $plugin_definition,
      $container->get('plugin.manager.entity'),
      $container->get('database'),
      $container->get('module_handler')
    );
  }

You now have the full advantages of OO and an IDE and when you want the entity manager, instead of calling

$entity_manager = $this->getContextValue('plugin.manager.entity');
$node_storage = $entity_manager->getStorageController('node');

You can call

$node_storage = $this->entityManager->getStorageController('node');

+1 context is for injecting user supplied context, like the language object in the language plugin, or an array of roles in the role plugin, not for services - that's my 2c™
I see context as equivalent to say arguments in a hook_rules_action_info definition. You define what context the user must provide, either by the UI or otherwise. Stuff that is non-negotiable like services etc are not the concern of the user, the plugin should interact with the container to receive these.

Yes, context allows you to inject both bits, and theoretically also use service names instead of using classes but what do we really gain?
People will write controllers and plugins very often, so consistency on how to inject the services is a win.

Let me see if I can articulate what I *think* EclipseGc's point here is, and thereby formulate a response.

The plugin system at its core is Drupal agnostic. This is a good thing. That means it also has no inherent coupling to the Symfony DependencyInjection component. This is also a good thing.

In practice, though, plugin instances have dependencies. In particular, there are 2 things they depend on:

1. Runtime contextual information (a node being viewed, current user, etc.)
2. Other services (database connection, the breadcrumb service, guzzle, entity manager, etc.)

Type 1 is handled by the Context portion of the API, which is, really, just a bunch of wrappers and generic methods to set stuff by name. It works well enough for that, and I don't think there's a debate there.

The tricky part is type 2; Kris' argument is "it's not tricky, just use the same generic API, problem solved." The counter-point is "providing services like that is what the DIC is for, damn well use it."

To have a concrete example, suppose we have a key/value lookup plugin type. At runtime you give it a string, and it returns some other string. We have two implementations:

1. This implementation requires a Guzzle connection as a service, a base URL as a configuration-based context, and a string at runtime. It will build the URL from the base and runtime string, request it through Guzzle, and return the value out of the response.
2. This implementation requires a DBTNG connection and will look up the value based on the supplied key from some known DB table. (In practice you'd probably use the new K/V system, but bear with me here.)

Nothing about implementation A is inherently tied to Drupal; Guzzle and strings are not Drupal-specific, so the plugin itself shouldn't be either. Keeping that decoupled from Drupal is a worthy goal, and one that we've been pushing for in many places.

However, it's not the plugin implementation that is the issue; it's the plugin manager that instantiates it. That needs to:

1. Know that this particular plugin implementation needs a Guzzle connection.
2. Know how to pass a Guzzle connection object to it. (Constructor, setter injection, whatever.)
3. Have a Guzzle connection sitting around to pass to it.

While the plugin can indicate its need for Guzzle as a constructor parameter (constructor injection), that doesn't provide enough information for the manager to know at runtime what object to give it. It could specify it via an annotation, but in what format? How does the manager know that it needs a guzzle object (or, rather, something implementing Guzzle's connection interface) to pass into it? And, more pointedly, once it knows that then how does it know where to get the connection from itself?

I do not see how re-using the context API as a form of generic setter injection solves that problem; the manager still needs to get a guzzle object from somewhere, and needs to know that it needs to do so.

Additionally, consider implementation B. It needs a DB connection, specifically a Drupal DBTNG connection. Right there, it's dependent upon Drupal (or at least on DBTNG). At that point independence from Drupal becomes largely theoretical. Now, I am all for theoretical purity; I've argued for it many times quite loudly. :-) But as a practical matter, I don't see how this theoretical purity is actually achieved.

So the part that I'm missing, and have always been missing, is that using Context API as a generic setter injection does not in fact divorce the entire plugin system from Drupal; It only divorces the plugin implementation, mostly, but not the plugin manager. The plugin manager is, 90% of the time, probably still container aware because otherwise I have no idea how you would implement the example above. And if merging service injection into Context API doesn't actually get us the Drupal-decoupling we want (and I think we all want that at this point, to the maximum degree feasible), then why shoe-horn those two systems together in the first place?

That is, Plugins as a concept are Drupal-agnostic, but a specific application of them (pretty much anyone that is going to have dependent services) in many cases cannot be. That plugin type would be coupled to some mechanism of acquiring services to inject, be it the Symfony DIC (which couples it to Symfony, not Drupal, technically), Zend's DIC implementation (about which I know next to nothing other than it exists), a Pimple instance (if you were using, say, Silex), or whatever.

If such shoe-horning does in fact provide that complete decoupling in some way that I don't understand, Kris, please provide a trivial implementation of the above example in a way that is Drupal agnostic and DIC agnostic but still achieves the goal of plugin classes that are fully injected (either with Guzzle or a DB connection). We need to be seeing code here, not speaking in generic terms, because otherwise we're going to continue talking past each other. If you think this issue is the wrong place to do so, please put it somewhere else that we can link to and reference. (Feel free to copy as much of this comment as you'd like to explain the sample case.)

busy morning so I was only able to skim not give the full read i wanted. My concern at least with the example given is that configuration and plugin definitions are static information. Modifying them from the canonical values is questionable and something left to site builders to modify behaviors not to implement core interactions. If you want some sort of context things like that it should be an additional argument to the constructor. I see it as valid that the create method would populate that with objects from the container using values from configuration or plugin definitions though.

I don't think modules really make sense as context and should be their own constructor object but something like this.
$context['modules'] = $container->get($configuration['module_handler_key']);

To expand on that last sentenct a bit, in my mind the idea of context(at least for this argument) is configurable arguments for plugins. Things that aren't configurable like module lists we will use normal DI concepts and pass them to constructors using arguments. Configurable DI through context I could see either being arguments or maybe this context array concept.

As per our long irc discussion today I am re-rolling this to use a container aware factory, borrowing from timplunkett's patch here #1863816-123: Allow plugins to have services injected.
I have it almost done - just trying to figure out the test, will get back to it tomorrow...

So I went with a PHPUnit test in the end, which means it only tests the actual plugin itself, not the manager / factory integration.

This patch seems to contains bits of #1863816: Allow plugins to have services injected

+++ b/core/modules/system/lib/Drupal/system/Plugin/Core/Condition/RequestPath.phpundefined
@@ -0,0 +1,122 @@
+    return new static($container->get('request'), $container->get('alias_manager'), $container->get('path_matcher'), $configuration, $plugin_id, $plugin_definition);

Afaik the service is called "path.alias_manager.cached"

+++ b/core/modules/system/lib/Drupal/system/Plugin/Core/Condition/RequestPath.phpundefined
@@ -0,0 +1,122 @@
+   * @param \Symfony\Component\HttpFoundation\Request $request
+   *   The HttpRequest object representing the current request.
+   *
+   * @param \Drupal\Core\Path\AliasManagerInterface $alias_manager
+   *   An alias manager to find the alias for the current system path.
+   *
+   * @param \Drupal\Core\Path\PathMatcherInterface $path_matcher
+   *   A path matcher for matching the path against a pattern.

No reason for these empty lines.

http://drupalcode.org/project/drupal.git/commitdiff/75ee363 went in!

Awesome! I'll try and get this rerolled tonight.

Rerolled now that #1863816: Allow plugins to have services injected is in, fixed the problem with the other Condition API integration tests (not in the interdiff, it was just a bad use statement), and addressed #66, although using the 'path.alias_manager' service instead of 'path.alias_manager.cached' because it makes no difference in this case and the latter may be going away soon.

#71: 1921544-current-path-condition-71.patch queued for re-testing.

+++ b/core/lib/Drupal/Core/Path/PathMatcher.php
@@ -0,0 +1,63 @@
+class PathMatcher implements  PathMatcherInterface {

Double space here.

Leaving at needs review. Looks good otherwise, but I didn't really look very thoroughly.

Re-rolled since #1903346: Establish a new DefaultPluginManager to encapsulate best practices went in. Fixed spacing issue in #74.

I ma not going to RTBC again after 68 comments a lot has changed but IMO it is ready.

The code itself is good, just some docs/standards stuff:

+++ b/core/includes/path.incundefined
@@ -38,27 +38,13 @@ function drupal_is_front_page() {
+ * @deprecated as of Drupal 8.0
+ * @see \Drupal\Core\Path\PathMatcherInterface::matchPath()

I don't think we use @see after @deprecated, the second line should be indented two spaces (like @todo) and should read more like a sentence

+++ b/core/includes/path.incundefined
@@ -38,27 +38,13 @@ function drupal_is_front_page() {
+  $path_matcher = \Drupal::service('path_matcher');
+  return $path_matcher->matchPath($path, $patterns);

Might as well be a oneliner

+++ b/core/lib/Drupal/Core/Path/PathMatcher.phpundefined
@@ -0,0 +1,63 @@
+  protected $regexps;

We use $regex everywhere, not $regexp, so let's make this $regexes (I honestly thought it was a typo when I read this.

+++ b/core/lib/Drupal/Core/Path/PathMatcher.phpundefined
@@ -0,0 +1,63 @@
+   * @param ConfigFactory $config_factory

Needs the full class name.

+++ b/core/lib/Drupal/Core/Path/PathMatcherInterface.phpundefined
@@ -0,0 +1,24 @@
+<?php
+/**

Missing blank line.

+++ b/core/lib/Drupal/Core/Path/PathMatcherInterface.phpundefined
@@ -0,0 +1,24 @@
+   * Check if a path matches any pattern in a set of patterns.

Checks

+++ b/core/lib/Drupal/Core/Path/PathMatcherInterface.phpundefined
@@ -0,0 +1,24 @@
+   * @return Boolean

@return bool

+++ b/core/lib/Drupal/Core/Path/PathMatcherInterface.phpundefined
@@ -0,0 +1,24 @@
+   *   value: TRUE if the path matches a pattern, FALSE otherwise.

Drop the "value: " part.

+++ b/core/modules/system/lib/Drupal/system/Plugin/Core/Condition/RequestPath.phpundefined
@@ -0,0 +1,120 @@
+  public static function create(ContainerInterface $container, array $configuration, $plugin_id, array $plugin_definition) {
...
+  public function __construct(Request $request, AliasManagerInterface $alias_manager, PathMatcherInterface $path_matcher, array $configuration, $plugin_id, array $plugin_definition) {

Swap the order of these two

+++ b/core/modules/system/lib/Drupal/system/Plugin/Core/Condition/RequestPath.phpundefined
@@ -0,0 +1,120 @@
+    return new static($container->get('request'), $container->get('path.alias_manager'), $container->get('path_matcher'), $configuration, $plugin_id, $plugin_definition);

Split these on multiple lines

+++ b/core/modules/system/lib/Drupal/system/Plugin/Core/Condition/RequestPath.phpundefined
@@ -0,0 +1,120 @@
+   *   A path matcher for matching the path against a pattern.

Missing a bunch of @params

Fixes code style/doc issues in #77

Thanks, code looks good!

Should we be moving towards matching the route and params instead of the path?

The problem is that users maybe will not have access to the route names. It seems to be way to technical for them.

Users won't have access to the route names - but the whole point of having a router + generator is being able to move routes around without path-specific code breaking. I think this could use more discussion - i.e. should we open a follow-up to add a route condition?

A good step would be switching the front page variable to actually store a route name + params? In that case and others I guess we'd find the route based on the path when the setting is saved?

I am wondering whether we can change APIs once something is in core, at this state?

#78: 1921544-current-path-condition-78.patch queued for re-testing.

tagging Needs reroll

Re-roll of #78

Created #2071553: Create route and params condition for #82. RTBC as per #79.

#88: 1921544-current-path-condition-88.patch queued for re-testing.

#88: 1921544-current-path-condition-88.patch queued for re-testing.

Patch no longer applies.

Rerolled.

Fixed a few code style issues.

The patch applies clean but fails phpunit with:

Cannot use Drupal\Component\Plugin\ContextAwarePluginBase as PluginBase because the name is already in use in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Plugin/ContextAwarePluginBase.php on line 10

I think I narrowed it down to the fact that Drupal\Core\Plugin\ContextAwarePluginBase.php does:

namespace Drupal\Core\Plugin;

use Drupal\Component\Plugin\ContextAwarePluginBase as PluginBase;

Fixed both in the attached patch.
and that we already have a PluginBase in Drupal\Core\Plugin\PluginBase.php

Another thing: Shouldn't the RequestPath condition annotate itself as a condition?

/**
 * Provides a 'Request Path' condition.
 *
 * @Condition(
 *   id = "request_path",
 *   label = @Translation("Request Path"),
 * )
 */
class RequestPath extends ConditionPluginBase implements ContainerFactoryPluginInterface {

#95: 1921544-current-path-condition-95.patch queued for re-testing.

1. +++ b/core/lib/Drupal/Core/Path/PathMatcherInterface.php
   @@ -0,0 +1,25 @@
   +interface PathMatcherInterface {
   

   We might should add some docs on there.

2. +++ b/core/modules/system/lib/Drupal/system/Plugin/Core/Condition/RequestPath.php
   @@ -0,0 +1,138 @@
   +      return t('Do not return true on the following pages: @pages', array('@pages' => $pages));
   ...
   +    return t('Return true on the following pages: @pages', array('@pages' => $pages));
   

   We could also inject the string_translation service and wrap it with a t method.

3. +++ b/core/modules/system/tests/Drupal/system/Tests/Condition/RequestPathConditionTest.php
   @@ -0,0 +1,98 @@
   +    $alias_manager = $this->getMockBuilder('Drupal\Core\Path\AliasManager')
   +      ->disableOriginalConstructor()
   +      ->getMock();
   +
   

   Is there a reason why we don't mock the interface directly? AliasManagerInterface

4. +++ b/core/modules/system/tests/Drupal/system/Tests/Condition/RequestPathConditionTest.php
   @@ -0,0 +1,98 @@
   +    $path_matcher = new PathMatcher($config_factory_stub);
   +    $new_request = Request::create('http://test.com/my/pass/page');
   +    // The condition checks the system_path attribute of the request so we must
   +    // set that to the path we want to evaluate.
   +    $new_request->attributes->set('system_path', 'my/pass/page');
   +
   +    $condition = new RequestPath($new_request, $alias_manager, $path_matcher, array(), 'request_path', array());
   +    $condition->setExecutableManager($condition_manager);
   +
   +    $pages = "my/pass/page\r\nmy/pass/page2\r\nfoo";
   +    $condition->setConfig('pages', $pages);
   +
   +    // Check the first configured path and summary.
   +    $this->assertTrue($condition->evaluate(), 'The system_path my/pass/page passes.');
   +
   +    // Check the second configured path.
   +    $new_request->attributes->set('system_path', 'my/pass/page2');
   +    $this->assertTrue($condition->evaluate(), 'The system_path my/pass/page2 passes.');
   +
   +    // Check that a system path that is not included in the pages config, but
   +    // whose alias is, passes.
   +    $new_request->attributes->set('system_path', 'my/aliased/page');
   +    $this->assertTrue($condition->evaluate(), 'The system_path my/aliased/page passes.');
   +
   +    $new_request->attributes->set('system_path', 'my/pass/page3');
   +    $condition->setConfig('pages', 'my/pass/*');
   +    $this->assertTrue($condition->evaluate(), 'The system_path my/pass/page3 passes for wildcard paths.');
   +
   +    // Check that a system path that is not included in the pages config, either
   +    // directly or via an alias, fails.
   +    $new_request->attributes->set('system_path', 'my/fail/page4');
   +    $this->assertFalse($condition->evaluate(), 'The system_path my/fail/page4 fails.');
   

   It seems to make sense to split up these tests into multiple test methods.

Thanks dawehner!

Not sure if this plugin is still relevant post api freeze? How will it be used?

This does everything suggested in #98 except for 4). I wanted to see if tests passed before splitting them all up.

Forgot to include the Interface name in the mock. Tests pass locally.

This takes care of splitting out the test methods as per #98.

+++ b/core/modules/system/tests/Drupal/system/Tests/Condition/RequestPathConditionTest.php
@@ -0,0 +1,136 @@
+/**
+ * Tests path processor functionality.
+ */
+class RequestPathConditionTest extends UnitTestCase {
+

Let's add a @group Drupal and maybe an @see of the tested class.

Good idea. I also cleaned up the imports.

Thank you!

I just had a short look at it, and it looks mostly good. However:

1. +++ b/core/lib/Drupal/Core/Path/PathMatcherInterface.php
   @@ -0,0 +1,28 @@
   +   *   String containing a set of patterns separated by \n, \r or \r\n.
   

   Thats seems weird to me. If it's multiple patterns, why isn't it an array of patterns?

   The parsing of the condition configuration string is surely not the responsibility of the general-purpose path matcher service?

2. +++ b/core/modules/system/lib/Drupal/system/Plugin/Core/Condition/RequestPath.php
   @@ -0,0 +1,159 @@
   +    // Convert path to lowercase. This allows comparison of the same path
   +    // with different case. Ex: /Page, /page, /PAGE.
   +    $pages = Unicode::strtolower($this->configuration['pages']);
   

   Is this something the path matcher should handle? Are paths with different capitalization generally speaking equal?

Setting to needs work for the 1.

Fago you're right about 1. And the units tests are all really testing PathMatcher. I'll have a go at moving them all over to a PathMatcher unit test.

Not sure about 2.

I've created a new phpunit test class for PatchMatcher and copied the tests over there. This means we can remove some of the tests from the RequestPath condition plugin, but need to check which ones.

Also, I've updated the matchPath() interface to take an array, which I think makes a lot more sense for an interface. Got lost in regex land so ended up just imploding on newline and re-using the existing code. Would be great if someone could offer suggestions on how to improve this. :-)

1. +++ b/core/modules/system/lib/Drupal/system/Plugin/Core/Condition/RequestPath.php
   @@ -0,0 +1,161 @@
   +    $pages = array();
   +    foreach (explode("\n", $this->configuration['pages']) as $page) {
   +      $pages[] = trim($page);
   +    }
   

   This can be collapsed to:

   $pages = array_map('trim', explode("\n", $this->configuration['pages']));

2. +++ b/core/modules/system/lib/Drupal/system/Plugin/Core/Condition/RequestPath.php
   @@ -0,0 +1,161 @@
   +    // Convert path to lowercase. This allows comparison of the same path
   +    // with different case. Ex: /Page, /page, /PAGE.
   +    $pages = Unicode::strtolower($this->configuration['pages']);
   

   For case-insensitivity questions, see: #2075889: Make Drupal handle incoming paths in a case-insensitive fashion for routing

I think the only reason drupal_match_path() took a string was that the block config form saved patterns as a big string, and rather than build an API we just built a utility function that we pretended was an API. :-) Array makes more sense here.

Fixes 1) in #112

113: 1921544-current-path-condition-112.patch queued for re-testing.

Taggging

No changes just fixes the service conflict.

Kick the bot

117: 1921544-current-path-condition-117.patch queued for re-testing.

117: 1921544-current-path-condition-117.patch queued for re-testing.

The service definition "path_matcher" does not exist.

Re-added missing service definition.

Drush installer works, but UI installer fails.

Need to manually add PathMatcher in the installer.

1. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
   @@ -92,6 +101,7 @@ public function __construct(StorageInterface $storage, EventDispatcher $event_di
        $this->language = $language;
   +    $this->preloadEventFired = FALSE;
      }
    
      /**
   @@ -184,6 +194,11 @@ public function get($name) {
   
   @@ -184,6 +194,11 @@ public function get($name) {
      public function loadMultiple(array $names) {
        global $conf;
    
   +    if (!$this->preloadEventFired) {
   +      $names = array_unique($names + $this->getPreloadNames());
   +      $this->preloadEventFired = TRUE;
   +    }
   +
        $list = array();
    
        foreach ($names as $key => $name) {
   @@ -262,6 +277,18 @@ protected function loadModuleOverrides(array $names) {
   
   @@ -262,6 +277,18 @@ protected function loadModuleOverrides(array $names) {
      }
    
      /**
   +   * Get a list of configuration object names to preload.
   +   *
   +   * @return array
   +   *   An array of configuration object names to preload.
   +   */
   +  protected function getPreloadNames() {
   +    $config_preload_event = new ConfigPreloadEvent();
   +    $this->eventDispatcher->dispatch('config.preload.names', $config_preload_event);
   +    return $config_preload_event->getNames();
   +  }
   +
   

   You seemed to have merged in another patch (preload config objects).

2. +++ b/core/lib/Drupal/Core/Path/PathMatcher.php
   @@ -0,0 +1,66 @@
   +  public function matchPath($path, array $patterns) {
   

   @EclipseGC We do support actual a different usecase here. This does not directly interfere with routing.

3. +++ b/core/lib/Drupal/Core/Path/PathMatcherInterface.php
   @@ -0,0 +1,28 @@
   +   * @param string[] $patterns
   

   <3

4. +++ b/core/modules/system/lib/Drupal/system/Plugin/Core/Condition/RequestPath.php
   @@ -0,0 +1,158 @@
   +      $container->get('path.alias_manager'),
   

   We should rather use path.alias_manager.cached instead.

1. Un-tangles the config preloading patch. Sorry!
2. No change?
3. :-)
4. Fixes

Inventing a new tag.

Should we move out the PathMatcherInterface into a separate issue?

Re-rolled against a load of changes. Also, This is built on top of #2268801: Update ConditionInterface to use recently added plugin interfaces.

Discussed with @tim.plunket in IRC and agreed to remove the PathMatcher interface and service and make it a followup.

Tests need to use RequestStack too.

Re-roll since #2268801: Update ConditionInterface to use recently added plugin interfaces went in.

Since we've gone back to calling drupal_match_path, I've stubbed that out of our unit tests so we don't need to call it.

138: 1921544-current-path-condition-138.patch queued for re-testing.

Need to set match to true by default.

Looks good overall. Just some things leftover because the issue is so old:

1. +++ b/core/modules/system/lib/Drupal/system/Plugin/Core/Condition/RequestPath.php
   @@ -0,0 +1,164 @@
   + *   module = "system"
   ...
   +  /**
   +   * Translates a string to the current language or to a given language.
   +   *
   +   * See the t() documentation for details.
   +   */
   +  protected function t($string, array $args = array(), array $options = array()) {
   +    return $this->translationManager->translate($string, $args, $options);
   

   Not needed anymore, use the trait.

2. +++ b/core/modules/system/tests/Drupal/system/Tests/Condition/RequestPathConditionTest.php
   @@ -0,0 +1,172 @@
   +    $alias_manager = $this->getMockBuilder('Drupal\Core\Path\AliasManagerInterface')
   +      ->getMock();
   

   Can't this just be getMock()?

3. +++ b/core/modules/system/tests/Drupal/system/Tests/Condition/RequestPathConditionTest.php
   @@ -0,0 +1,172 @@
   +    $translation_manager = $this->getMockBuilder('Drupal\Core\StringTranslation\TranslationInterface')
   +      ->getMock();
   

   $this->getStringTranslationStub()

Thanks Tim. Fixes for #143. Also since we are not calling drupal_match_path anymore, removed stub config that it used.

I've split off the PathMatcher service to #2272201: Move drupal_match_path in path.inc to a service

Hi Eclipse,

I've actually stubbed out the call to drupal_match_path in the unit tests. However, I take your point on needing an integration test. Hopefully just extending KernelTestBase.

K

ContextAwarePluginBase is already using that trait.

I've moved the plugin to the correct namespace, and moved all tests over to a new test extending KernelTestBase so we are now interacting with drupal_match_path etc.

I've also made the request a context parameter, as this makes it a lot easer for testing, and keeps it in line with how context is being used in the other plugins.

That looks much better.

1. +++ b/core/modules/system/lib/Drupal/system/Plugin/Condition/RequestPath.php
   @@ -0,0 +1,138 @@
   + *   module = "system",
   

   I think this can go

2. +++ b/core/modules/system/lib/Drupal/system/Plugin/Condition/RequestPath.php
   @@ -0,0 +1,138 @@
   +  protected $aliasManager;
   +
   +
   +  /**
   

   Extra line here

3. +++ b/core/modules/system/lib/Drupal/system/Plugin/Condition/RequestPath.php
   @@ -0,0 +1,138 @@
   +      '#default_value' => !empty($this->configuration['pages']) ? $this->configuration['pages'] : '',
   

   You should implement defaultConfiguration(), provide '' as a default, and skip the !empty check

4. +++ b/core/modules/system/lib/Drupal/system/Plugin/Condition/RequestPath.php
   @@ -0,0 +1,138 @@
   +          '%user' => 'user',
   +          '%user-wildcard' => 'user/*',
   +          '%front' => '<front>',
   +        )),
   

   These are indented too far. I know phpstorm likes to do that

Thanks @tim.plunkett! Fixes for #152.

Excellent! Thanks @kim.pepper!

Tests are passing after PSR-4 re-roll. Putting back to RTBC.

hm, I must say I'm unsure about the context specified as class only. How would someone make use of it and provide the context in a general fashion?

I guess I asked my the same. I see a very good use case here in general for block visibility and code visibility in Google Analytics. I'm also using the same logic like block visibility, but it would be great if the code is maintained only once in core and I can just reuse it and get rid of _google_analytics_visibility_pages() and maybe _google_analytics_visibility_roles(), too.

hm, I must say I'm unsure about the context specified as class only. How would someone make use of it and provide the context in a general fashion?

The general approach we've discsussed in IRC is to put anything that is being evaluated in context (e.g. request, user role, etc.). You could inject the request stack, but I thinks this makes the usage pattern a lot clearer. See the tests for examples of how you would pass it in.

You could inject the request stack, but I thinks this makes the usage pattern a lot clearer

Think about it, I think that is what makes sense to me. Either, it's just a dependency or it's required context which the user needs to be able to configure. If it's a dependency, it should be injected, if it should be conifugrable we should have sufficient metadata for the context needed, i.e. a type, label etc.

Setting back to needs work as this needs more discussion.

Fago and I discussed this at Drupalcon. I guess I don't really understand context, and it appears that this isn't the right usage.

I've reverted back to using a request_stack.

Created a follow up #2278541: Refactor block visibility to use condition plugins

The change looks good to me. As discussed with eclipsegc one can argue whether a request should be defined as context, but as said above - if so we should use our regular way to define it. I'm fine with adding this without a request context.

+++ b/core/modules/system/src/Tests/Plugin/Condition/RequestPathTest.php
@@ -0,0 +1,134 @@
+   * Test the user_role condition.

outdated comment.

Great! Fixes comments in #164.

ok, back to RTBC then.

As per #2124749: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API we shouldn't longer rely on the _system_path request attribute. Is there a reason why we cannot use $request->getPathInfo() here?

Until #2239009: Remove public direct usage of the '_system_path' request attribute goes in, this is still fine.

Still we could use $request->getPathInfo() and not introduce another _system_path dependency in the first place. Was this considered at any point?

Please ignore my comments. I had the impression that this introduces a new dependency on the _system_path request attribute. Instead the changes here only make the existing dependency more explicit, which is a good thing.

Committed acb97e8 and pushed to 8.x. Thanks!

Quick follow-up.
#2283061: RequestPath condition uses non-existent service path.alias_manager.cached