Problem/Motivation

We want to remove include files (including path.inc) and move from procedural to OO code.

Proposed resolution

Move drupal_valid_path() to a PathValidator service.

Remaining tasks

RTBC

User interface changes

None

API changes

drupal_valid_path() becomes deprecated

CommentFileSizeAuthor
#3 interdiff.txt1.15 KBkim.pepper
#3 2302331-valid-path-3.patch6.35 KBkim.pepper
#1 2302331-valid-path-1.patch6.28 KBkim.pepper

Comments

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Title: Move drupal_valid_path to PathMatcher service » Move drupal_valid_path to PathValidator service
Status: Active » Needs review
StatusFileSize
new 2302331-valid-path-1.patch6.28 KB

Created a new PathValidator service and moved over the logic of drupal_valid_path().

ParisLiakos’s picture

ParisLiakos commented
Issue summary: View changes

Looks good. All this needs is a change notice and fixing those nitpicks :) :

  1. +++ b/core/lib/Drupal/Core/Path/PathValidator.php
@@ -0,0 +1,116 @@
+namespace Drupal\Core\Path;
+use Drupal\Component\Utility\UrlHelper;

    newline in between 

  2. +++ b/core/lib/Drupal/Core/Path/PathValidator.php
@@ -0,0 +1,116 @@
+ * @todo add class comment.

    :) 

  3. +++ b/core/lib/Drupal/Core/Path/PathValidatorInterface.php
@@ -0,0 +1,27 @@
+interface PathValidatorInterface {
+
+
+  /**

    Too many newlines here

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
StatusFileSize
new 2302331-valid-path-3.patch6.35 KB
new interdiff.txt1.15 KB

Fixes for #2.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

Added draft change record here: https://www.drupal.org/node/2302541

ParisLiakos’s picture

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

thanks

+++ b/core/lib/Drupal/Core/Path/PathValidator.php
@@ -0,0 +1,117 @@
+ * Provides a default path validator and access checker.

dont really like the "default" in here, but OK

change notice looks good

chx’s picture

chx commented

I needed to review this because path validation is security related. And this of course is good (or not worse than HEAD) and should could be committed as it.

I moved my concerns to #2302563: Access check Url objects

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 291b998 and pushed to 8.x. Thanks!

diff --git a/core/includes/path.inc b/core/includes/path.inc
index ff63a6d..fc7f5fb 100644
--- a/core/includes/path.inc
+++ b/core/includes/path.inc
@@ -5,9 +5,7 @@
  * Functions to handle paths in Drupal.
  */
 
-use Drupal\Component\Utility\UrlHelper;
 use Drupal\Core\ParamConverter\ParamNotConvertedException;
-use Drupal\Core\Routing\RequestHelper;
 use Symfony\Cmf\Component\Routing\RouteObjectInterface;
 
 /**

Fixed on commit.

  • alexpott committed 291b998 on 8.x 
    Issue #2302331 by kim.pepper: Move drupal_valid_path to PathValidator...

Status: Fixed » Closed (fixed)

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