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 CreditAttribution: herom commentedComment #6
martin107 CreditAttribution: 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 CreditAttribution: martin107 commentedThis should fix things
Comment #10
dawehnerthank you so much for rerolling this patch!
Comment #11
martin107 CreditAttribution: martin107 commentedRemoved unused use statement.
Comment #12
Crell CreditAttribution: 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 CreditAttribution: 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\Component
components MUST NOT depend onDrupal\Core
components or any other Drupal code. But aside from that, anything that can be specified in a hypotheticalcomposer.json
file 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 CreditAttribution: znerol commentedComment #20
Crell CreditAttribution: 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 CreditAttribution: 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 CreditAttribution: Ivan Berezhnov as a volunteer and at Drupal Ukraine Community for Levi9 commentedComment #33
vsujeetkumar CreditAttribution: vsujeetkumar at Material for Drupal India Association commentedComment #34
vsujeetkumar CreditAttribution: vsujeetkumar at Material for Drupal India Association commentedComment #35
prabha1997 CreditAttribution: prabha1997 at Valuebound for Valuebound commentedI am not able to reroll a patch [#11 patch]
Comment #36
saurabh-2k17 CreditAttribution: saurabh-2k17 at Material for Drupal India Association commentedI am getting this for Patch #11
git apply --check routing-2296205-11.patch
Comment #37
martin107 CreditAttribution: martin107 as a volunteer 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 CreditAttribution: martin107 as a volunteer commentedsite install now works.
Comment #40
martin107 CreditAttribution: martin107 as a volunteer commentedNot yet complete just posting while I work
Comment #41
martin107 CreditAttribution: martin107 as a volunteer 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 CreditAttribution: martin107 as a volunteer commentedComment #43
martin107 CreditAttribution: martin107 as a volunteer commentedMore tests pass.
Comment #44
martin107 CreditAttribution: martin107 as a volunteer commentedmore fixes.
Comment #45
martin107 CreditAttribution: martin107 as a volunteer 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 CreditAttribution: martin107 as a volunteer 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 CreditAttribution: martin107 as a volunteer 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 CreditAttribution: martin107 as a volunteer commentedMy bad, I mean this patch
Comment #52
martin107 CreditAttribution: martin107 as a volunteer commentedFixing tests
Comment #53
martin107 CreditAttribution: martin107 as a volunteer 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 CreditAttribution: martin107 as a volunteer commentednext steps
git rebase 9.0.x
the rebase to 9.1.x
Comment #55
shaktikRe-roll patch of #45
Comment #56
martin107 CreditAttribution: martin107 as a volunteer 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 CreditAttribution: martin107 as a volunteer 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 CreditAttribution: martin107 as a volunteer commentedAll these failures also appear to have a small common set of causes.
Comment #60
martin107 CreditAttribution: martin107 as a volunteer commentedThis is slowly coming good.
-use Drupal\comment\Routing\RouteMatchInterface;
is not a thing
Comment #61
martin107 CreditAttribution: martin107 as a volunteer 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 CreditAttribution: martin107 as a volunteer 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 CreditAttribution: martin107 as a volunteer commentedLooking at the stack trace
When I see
Class 'Drupal\Core\Routing\CurrentRouteMatch' not found
in 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 CreditAttribution: martin107 as a volunteer commentedI finally found the time to unclog this.
Comment #69
martin107 CreditAttribution: martin107 as a volunteer 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.