Problem/Motivation
The new route match is a concept which is not bound to drupal itself. On the other hand it is a great tool to decouple your
code from needing the request in all kind of places.
On the other hand this makes code less compatible with other PHP folks code out there.
Proposed resolution
Move the Route match into the \code>\Drupal\component\Routingnamespace, so one day it can be shared using composer.
Remaining tasks
User interface changes
API changes
Original report by @effulgentsia
Did we actually ever considered to move the routeMatch interface into Drupal/Component? This could be really useful for other code as well and allows people to write generic libraries without pulling in all of drupal.
| Comment | File | Size | Author |
|---|---|---|---|
| #67 | interdiff-2296205-61-67.txt | 585 bytes | martin107 |
| #67 | 2296205-67.patch | 241.51 KB | martin107 |
| #60 | 2296205-60.patch | 240.13 KB | martin107 |
| #59 | interdiff-2296205-57-59.txt | 1.84 KB | martin107 |
| #59 | 2296205-59.patch | 240.13 KB | martin107 |
Comments
Comment #1
dawehnerThanks for creating the issue!
Comment #2
dawehnerHere is a list of the available bits, just in Core/Routing:
TL;DR: The route match is quite useful, other ones not really.
Comment #3
dawehnerDrupal 8 installed with this.
Comment #4
dawehnerTry to learn things again.
Comment #5
herom commentedComment #6
martin107 commentedThis issue seems like a good idea, so :-
Reroll.. had to update \Drupal\shortcut\Form\SwitchShortcutSet to get it to install, I think testbot will expose others which need updating.
Comment #8
dawehnerComment #9
martin107 commentedThis should fix things
Comment #10
dawehnerthank you so much for rerolling this patch!
Comment #11
martin107 commentedRemoved unused use statement.
Comment #12
Crell commentedWhile I'm generally in favor of more Drupal-agnostic code, this is all code that depends on Symfony's Routing component either directly or indirectly. The rules for Drupal\Component are currently *no* dependencies outside of Drupal\Component, and preferably not even then. So this wouldn't be moveable right now unless we change the policy for Drupal\Component.
Comment #13
dawehnerI am sorry but this is just wrong in reality:
Comment #14
Crell commentedWell, then people haven't been reading their README files:
http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Component/README.txt
Comment #15
sunThose examples are perfectly in line with that README.txt. If the README text was supposed to be interpreted differently, then it (1) should have used unambiguous wording and (2) would have to include a rationale for why such an extreme stance of not depending on anything else at all is a good idea in any way.
My interpretation of that README.txt is as simple as this:
Drupal\Componentcomponents MUST NOT depend onDrupal\Corecomponents or any other Drupal code. But aside from that, anything that can be specified in a hypotheticalcomposer.jsonfile of the component is a legit, appropriate, and possibly even desired dependency.Comment #16
dawehnerOpened a different issue as it is independent from this issue: #2303777: Allow drupal components to depend on other components outside Drupal
Comment #17
dawehnerSo given that we just need an update to README.txt can we move this issue forward?
Comment #18
dawehner@crell
There is simply no reason on earth to block that given that also #1972300: Write a more scalable dispatcher introduced a new class in there which does depends on an external component.
Comment #19
znerol commentedComment #20
Crell commented*sigh*. Fine. Because godforbid we actually follow our own written rules and policies. It's so much easier to just conveniently ignore them rather than changing them explicitly.
Comment #21
effulgentsia commentedWhy is this Major priority?
While I think this is a nice thing to do, I don't think it brings enough benefit to Drupal 8.0.0 to justify spending more time on it prior to D8's release and breaking other patches in the queue. How about postponing it to 8.1? It'll mean needing to retain the \Core classes as wrappers for BC, but at this point in the 8.0 cycle, we would need to do that for 8.0 as well.
Comment #22
dawehner@effulgentsia
You know sometimes sharing with other people seemed to be a good idea, though I don't remember why I set it to major. Maybe
this was a moment of rage. I do agree that moving to normal and 8.1.x is the right idea.
Comment #27
dawehnerI think noone cares about this task anymore. Any objections against just closing down this issue?
Comment #28
ivan berezhnov commentedComment #33
vsujeetkumar commentedComment #34
vsujeetkumar commentedComment #35
prabha1997 commentedI am not able to reroll a patch [#11 patch]
Comment #36
saurabh-2k17 commentedI am getting this for Patch #11
git apply --check routing-2296205-11.patch
Comment #37
martin107 commentedIt is good to activity on this issue ... Here is a reroll.
With over 130 CONFLICTing files in the reroll ... I expect a few error. which I will fix.
Comment #38
martin107 commentedsite install now works.
Comment #40
martin107 commentedNot yet complete just posting while I work
Comment #41
martin107 commentedTo speak to Crell's objections....
automated tests now, a few years later, are present which ensure that component files no longer depend on Core objects.
Additionally other test enforce the presence of
LICENSE.txt and README.txt files.
which I have added here.
As a note for myself linting of files can be done in this command
When this come back green, I am going to test against 9.1.x which is where I think this need to be
Comment #42
martin107 commentedComment #43
martin107 commentedMore tests pass.
Comment #44
martin107 commentedmore fixes.
Comment #45
martin107 commentedRegarding the failing tests
RouteMatchTest and CurrentRouteMatchTest
through RouteMatchTestBase they depend on Drupal\Tests\UnitTestCase
which is a prohibited core object ( given the conversion )
The actual dependence is fake, so we can just drop back to depending on the lesser PHPUnit\Framework\TestCase
A happy accident.
Comment #46
martin107 commentedvsujeetkumar, prabha1997 or Saurabh_sgh
if you want to work on this ... I guide you through tasks.
Comment #48
shaktikHi @martin107,
Patch #45 no longer to apply on 8.9 and 9.1 need re-reroll wokinrg on it.
Comment #49
martin107 commentedThis reroll is a tricky.
The original patch was developed on the 8.9.x branch
The currently active branch 9.1.x is so different that the patch cannot be rerolled using the conventional method.
So the plan is in 2 stages .. first reroll to the head of 8.9.x and then we will proceed from there.
Comment #50
martin107 commentedMy bad, I mean this patch
Comment #52
martin107 commentedFixing tests
Comment #53
martin107 commentedThe last interdiff is correct -- it show the changes I made.
BUT the last patch was diffed against 9.1.x ( 8.9.x was intended )
More haste less speed.
Comment #54
martin107 commentednext steps
git rebase 9.0.x
the rebase to 9.1.x
Comment #55
shaktikRe-roll patch of #45
Comment #56
martin107 commentedNice work shaktik .. thanks for the reroll... that was a really big task.
There is a minor fix which seems to be a the centre of all the failing tests. Here is the fix.
Comment #57
martin107 commentedThat reduced the failure count ... alot .. but not perfectly
This time, I did a search replace on
use Drupal\Core\Routing\RouteMatchInterface;
Comment #59
martin107 commentedAll these failures also appear to have a small common set of causes.
Comment #60
martin107 commentedThis is slowly coming good.
-use Drupal\comment\Routing\RouteMatchInterface;
is not a thing
Comment #61
martin107 commentedI have fixed the obvious failing test
The one remaining functional test will take some detailed bug hunting ... I will take a look on Sunday.
Comment #63
tim.plunkettAlso note this issue: #2917331: Decouple from Symfony CMF
Comment #64
martin107 commentedHmm ...
I ran out of time this evening ..
The only useful thing I have to add is to describe test failure.
What is asserted is that the 403 page is generated using the seven theme but after debugging...
The problem is the the expected page is correctly returned but what was returned was built using the stable theme not the seven theme...
Last time we had passing tests it was on the 8.9.x branch
So I need to understand how the theme selection has changed between D8 and D9. Any suggestions welcome
Comment #65
shaktikHi @martin107,
getting error after applied #61, Kindly check.
Comment #66
martin107 commentedLooking at the stack trace
When I see
Class 'Drupal\Core\Routing\CurrentRouteMatch' not foundin relation toa call to createService()
->createService(Array, 'current_route_match')It makes me think that the definition for current_route_match in core.services.yml is invalid. but the patch looks correct.
infact searching all files in my core directory tells me the legacy string "Drupal\Core\Routing\CurrentRouteMatch" does not exists
so I think something in your environment did not get rebuilt correctly
Can I ask you to press the "Clear all caches" button - that should invalidate the kernel
as it calls
drupal_flush_all_caches() which has this line
Locally I can reproduce the failing test after a clear cache. ( testbot report 2 failing test .. but it can't count)
Comment #67
martin107 commentedI finally found the time to unclog this.
Comment #69
martin107 commentedHa, when I test locally ... I can get all listed failing tests to pass.... I am retesting
Comment #70
shaktikThanks, @martin107 it's working fine after the cache clear of #61 patch and finally, issue fixed.
Comment #71
shaktikComment #72
tim.plunkettThis is looking pretty good. But I believe this should be postponed on the CMF issue, as that adds new classes to \Drupal\Core\Routing.