Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
We are converting most of core to OOP. drupal_match_path is procedural and is called from a lot of OO code. This makes that code difficult to unit test.
Proposed resolution
Create a new PathMatcher service and move procedural code from path.inc to the service. Mark procedural code as deprecated, and do conversions in a follow up.
Remaining tasks
Do it.
User interface changes
None.
API changes
drupal_match_path in path.inc are marked deprecated.
Follow ups
Comment | File | Size | Author |
---|---|---|---|
#33 | interdiff.txt | 3.47 KB | kim.pepper |
#33 | 2272201-path-service-33.patch | 17.53 KB | kim.pepper |
#30 | interdiff.txt | 9.48 KB | kim.pepper |
#30 | 2272201-path-service-30.patch | 14.06 KB | kim.pepper |
#27 | 2272201-path-service-27.patch | 8.03 KB | kim.pepper |
Comments
Comment #1
kim.pepperComment #2
kim.pepperInitial commit of a PathMatcher service. This only includes drupal_match_path for now.
Comment #3
larowlanMy 2c
Some spacing could be cleaned up here? Also sentence doesn't really make sense.
nitpick: I think this has to start with a verb, 'Provides an' maybe
these lend themselves to a provider?
Comment #4
kim.pepperThanks for the review!
Fixes for #3.
Comment #5
kim.pepperWhile there is very little code to cover, code coverage report looks good!
Comment #6
dawehnerIt would be great to not do anything in the constructor. Calling the config factory methods could deal with language overrides which could trigger the entity system and what not. This resulted in a couple of circular dependencies in the past, so we figure out that it is a bad pattern.
Nice!
Comment #7
dawehner:(
Comment #8
kim.pepperThanks for the review dawehner!
I've changed it so the configure front page gets called only when needed.
Comment #9
dawehnerThank you
Comment #10
hass CreditAttribution: hass commentedWhy are we supporting arrays and strings as patterns and not only arrays?
Comment #11
tim.plunkett@hass, open a feature request. This just moves the function to a method, we're not changing the expectations or internals.
Comment #12
hass CreditAttribution: hass commentedIt changes internals.
Comment #13
kim.pepper@hass is correct. This changes the method signature from taking a string to an array of strings, then joining them again.
Comment #14
kim.pepperThis rolls back the api change so matchPath just takes a single string of patterns separated by newline. This is now essentially just moving old code into a service.
Kim
Comment #16
kim.pepperMissed something in the rollback. Try again!
Comment #17
hass CreditAttribution: hass commented:-( i was happy about this array change... Just wished it will change consequently in all places of core... :-)
Comment #18
kim.pepper@hass Are you happy to mark this reviewed and make api changes in a follow-up?
Comment #19
ParisLiakos CreditAttribution: ParisLiakos commentedLooks good but needs a draft change notice:)
Comment #20
kim.pepperThanks @ParisLiakos. Added a draft change notice [#2274675]
Comment #21
ParisLiakos CreditAttribution: ParisLiakos commentedi liked the array version too, but lets do a straight conversion here and move the array change to a followup.
thanks @kim.pepper !
Comment #22
kim.pepperCreated followup #2274701: Change PathMatcher::matchPath() $patterns param from string to an array of strings
Comment #23
hass CreditAttribution: hass commentedSounds very good to me. Thx.
Comment #25
kim.pepperRe-roll after PSR4 got committed.
Comment #27
kim.pepperAnother re-roll
Comment #28
hass CreditAttribution: hass commentedLooks green
Comment #29
alexpottCan we remove
Drupal\system\Tests\Path\MatchPathTest
- and ensure that all the test cases that covers are covered by the phpunit test.Comment #30
kim.pepperI've removed
Drupal\system\Tests\Path\MatchPathTest
and copied all the tests in there over tocore/tests/Drupal/Tests/Core/Path/PathMatcherTest.php
Comment #31
jibranThank you. Back to RTBC.
Comment #32
alexpottWe can now inject this into the new RequestPath condition
Comment #33
kim.pepperInjected PathMatcher into the request path condition.
Comment #35
kim.pepper33: 2272201-path-service-33.patch queued for re-testing.
Comment #36
tim.plunkettEverything in here looks good. @kim.pepper++
Nice!
Comment #37
alexpottCommitted 9b821cd and pushed to 8.x. Thanks!