Problem/Motivation
Drupal\Core\Routing\RouteProvider lacks a routeExists method. There is a close method, getRouteByName, but it throws an exception.
Right now, the standard way of checking of a route exists, based on this answer on drupal.stackexchange.com, is this:
$exists = count($route_provider->getRoutesByNames(['abc.xyz'])) === 1;
https://drupal.stackexchange.com/a/222652/45409
It would be nice if RouteProvider had a public method such as this, or if that's not the best place, some other built in service.
Proposed resolution
It would be useful to have a method added to RouteProvider that doesn't throw an exception, called ::routeExists perhaps.
Remaining tasks
- review
Draft Record Change:
https://www.drupal.org/node/3206861
Comment | File | Size | Author |
---|---|---|---|
#43 | 3029545-43.patch | 4.55 KB | quietone |
| |||
#43 | interdiff-36-43.txt | 2.17 KB | quietone |
#36 | interdiff-34-36.txt | 890 bytes | Liam Morland |
#36 | interdiff-27-34.txt | 3.07 KB | Liam Morland |
#36 | drupal-create_routeExists-3029545-36.patch | 4.55 KB | Liam Morland |
Issue fork drupal-3029545
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
oknateComment #3
oknateComment #5
oknateComment #6
oknateComment #11
mlncn CreditAttribution: mlncn at Agaric for Drutopia, Find It Cambridge, Cambridge, Massachusetts Family Policy Council commentedPatch works great. Nice API addition.
Comment #12
catchNeeds a re-roll and a change record.
Comment #14
oknateAdded reroll and draft record change:
https://www.drupal.org/node/3206861
Comment #16
mlncn CreditAttribution: mlncn at Agaric for Drutopia, Find It Cambridge, Cambridge, Massachusetts Family Policy Council commentedi can't push to the issue fork but here's another reroll as a patch
Comment #17
mlncn CreditAttribution: mlncn at Agaric for Drutopia, Find It Cambridge, Cambridge, Massachusetts Family Policy Council commentedBecause this re-roll affected only a comment (preserving "Tests…" instead of the older "Test"), setting back to RTBC.
Comment #18
catchThanks for the reroll, looked at the patch again and a couple more questions:
1. Do we need an interface with this method for RouteProvider to implement?
2. Should we add a return type hint here since it's new code?
Comment #19
Praveen Saini CreditAttribution: Praveen Saini as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedHi Catch, #18
I have reviewed the patch @#16 and it works great but it still needs some improvements.
For your queries
1. Do we need an interface with this method for RouteProvider to implement?
Yes, This method should be defined inside RouteProviderInterface. Its good to have it inside RouteProviderInterface as we can use it to extended the functionality across system.
2. Should we add a return type hint here since it's new code?
Type hint can help as it's a way of self documentation of method and also tightly restrict developers to write good quality code.
Comment #20
Praveen Saini CreditAttribution: Praveen Saini as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedHere is the patch according to #18
Comment #21
Praveen Saini CreditAttribution: Praveen Saini as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #22
Praveen Saini CreditAttribution: Praveen Saini as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #23
Praveen Saini CreditAttribution: Praveen Saini as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #24
Praveen Saini CreditAttribution: Praveen Saini as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #25
Praveen Saini CreditAttribution: Praveen Saini as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #27
Praveen Saini CreditAttribution: Praveen Saini as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #28
Praveen Saini CreditAttribution: Praveen Saini as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #32
smustgrave CreditAttribution: smustgrave at Mobomo commentedpublic function routeExists($name)
This needs to be typehinted both function and parameter
public function routeExists($name)
Parameter needs typehinting
All instances of that outside the tests.
Comment #34
Liam MorlandThis adds type declarations and uses casting instead of
count()
to check whether the array is empty.If the merge request is to to used, I think the person who created the merge request needs to update its target branch to point to 11.x.
Comment #36
Liam MorlandFix test.
Comment #37
smustgrave CreditAttribution: smustgrave at Mobomo commentedCR looks good, updated branch numbers.
Think this is a good addition.
Comment #39
nlisgo CreditAttribution: nlisgo as a volunteer commentedComment #40
nlisgo CreditAttribution: nlisgo as a volunteer commentedHitting retest to see if we can display a green run.
The random test failure has already been logged: #3376563: Random test fail in Drupal\Tests\Component\Utility\RandomTest::testRandomMachineNamesUniqueness
Comment #41
nlisgo CreditAttribution: nlisgo as a volunteer commentedComment #42
nlisgo CreditAttribution: nlisgo as a volunteer commentedSlight improvement, imo. Not a deal breaker for me.
Unless we anticipate a lot of calls to routeExists with an empty string then let's just assume the supplied string is a potential route and then explicitly declare true if not empty rather than casting.
Comment #43
quietone CreditAttribution: quietone at PreviousNext commentedI'm triaging RTBC issues.
I read the Issue summary, comments and the change record. I found that all questions have been answered. The CR is complete and has a before and after section which makes it easy for the reader. It is a bit brief for my tastes but it does not require a change.
I looked at the patch and ran the tests.
The summary line is to be 80 characters max. This could be changed by using @covers for the methods being tested.
And since a change is being made, this could use a simple comment to help the next person working on this.
I decided to make a patch for the above and at the same time I have added the suggestion in #42.
Comment #44
smustgrave CreditAttribution: smustgrave at Mobomo commentedAll green so back to RTBC