Yes, we're well past API freeze. That said...
Tonight I was futzing around with Symfony because what else are you going to do when you have insomnia? I discovered that all of the examples from the "official" Symfony docs for the routing component use "path" and not "pattern" when defining the URL of the route, and so it's going to look like some weird Drupalism that we use "pattern" there (even though I realize this was the old standard in Symfony 2.1 and below).
We could keep this backwards compatible by still allowing 'pattern', but I feel strongly we should switch the defaults in core anyway to match Symfony's docs to help eliminate newbie confusion. What say you?
Comment | File | Size | Author |
---|---|---|---|
#35 | path-pattern-2051097-34.patch | 110.51 KB | tim.plunkett |
#31 | drupal8.routing-system.2051097-31.patch | 121.49 KB | neclimdul |
#31 | interdiff.txt | 1.69 KB | neclimdul |
#29 | pattern-path-2051097-29.patch | 120.82 KB | dawehner |
#29 | interdiff.txt | 1.59 KB | dawehner |
Comments
Comment #1
dawehner+1 especially because we ask people to look at the docs of symfony if they want to understand the route system.
Technically we don't even have to switch a single line as symfony itself is backport compatible as you said. Accessing the pattern/path is technically the same, so what about creating maybe the biggest task in amount of actual changes :)
Comment #2
Manuel Garcia CreditAttribution: Manuel Garcia commentedExecuted from drupal root:
find ./ -name *.routing.yml -type f -exec sed -i 's/pattern:/path:/' {} \;
Patch includes the change from @dawehner on #1
Comment #4
naveenvalechaTested the patch #2 on simpletest.me and it shows error is due to this :
Comment #5
Manuel Garcia CreditAttribution: Manuel Garcia commentedI'm not realy sure if this is what we are supposed to be doing besides the changes on my previous patch, but here it is...
I've renamed getPatterns() method into getPath(), and updated every call that I saw in Drupal core files:
This patch also includes the changes from #2.
Comment #6
Manuel Garcia CreditAttribution: Manuel Garcia commentedComment #8
Manuel Garcia CreditAttribution: Manuel Garcia commentedHere is the error log from simplytest.me
The interesting part I assume is this:
SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'pattern' cannot be null: INSERT INTO {router} (name, route_set, fit, pattern, pattern_outline, number_parts, route) VALUES
I've looked into core/lib/Drupal/Core/Routing/MatcherDumper.php, and inside public function dump we are using getPath() to get the value to be inserted in the DB... So it looks ok to me. Any clues anyone?
This also raises the question about the table field 'pattern', should we rename it also?
Comment #9
Manuel Garcia CreditAttribution: Manuel Garcia commentedForgot to attach.
Comment #10
Manuel Garcia CreditAttribution: Manuel Garcia commentedInvalid file extension error page got rid of tags apparently...
Comment #11
naveenvalechaI think crell the maintainer of Routing system will know better.So Adding tags.
Comment #12
dawehner#5: drupal-pattern-path-routing-2051097-5.patch queued for re-testing.
Comment #14
tlattimore CreditAttribution: tlattimore commentedLooks like this patch has gotten stale so tagging.
Comment #15
tlattimore CreditAttribution: tlattimore commentedHere's a re-roll of the patch in #5. No interdiff of course because of the conflict.
Comment #17
dawehnerIt would be better to keep the old method, so there is no api change at all. getPattern could get a @deprecated tag so there is no API change needed.
Comment #18
webchickThis is probably at least major. catch says:
"major or critical, feels like an 'upgrade to latest symfony and don't use deprecated crap' issue."
so tagging.
Comment #19
webchickComment #20
dawehnerWell, we won't update to symfony > 2.3 anyway so at least for the 8.x release cycle we are totally safe.
Comment #21
amontero#15: 2051097-pattern-to-path-15.patch queued for re-testing.
Comment #23
Crell CreditAttribution: Crell commentedActually catch has indicated he wants to track Symfony 2.4, 2.5, etc., since they're going to be very careful with BC for the rest of the 2.x cycle.
I am fine with this change. There's a couple of tweaks that have been made to the route yaml file since we first started using it (all BC-able), which we should probably adopt. This is the big one.
I think it's still safe to rename the DB fields, in which case we should rename both pattern and pattern_outline.
And let's make sure that our code isn't bypassing whatever it is that supports both pattern and path in Symfony, since we do bypass/override various pieces of it.
Comment #24
catchYes I'd like to track 2.4, 2.5 if it's at all possible. Same goes for other PHP libraries we've pulled in as well as JavaScript.
I'm concerned about us eventually getting locked out of performance improvements and etc. because they're no longer backported to 2.3 (even if that's still LTS and has security support).
How realistic that is we won't know until we get there.
Comment #25
dawehnerRerole
Comment #27
dawehnerHa, a few places tried to hide.
Comment #29
dawehnerThis should be it.
Comment #31
neclimdulquick test fixes.
Comment #32
chx CreditAttribution: chx commented> we ask people to look at the docs of symfony if they want to understand the route system.
LOL
Comment #33
webchickCommitted and pushed to 8.x. Thanks!
Needs a change notice. I'll take care of it.
Also, I don't get what's so funny in #32. I don't understand why we'd duplicate all of the documentation at http://symfony.com/doc/current/book/routing.html.
Comment #34
webchickDone: https://drupal.org/node/2089727 and about 50 other change notices updated, too. :P
Tim noticed a bug with spaces in the patch so marking back to needs work.
Comment #35
tim.plunkettThis has an extra space everywhere. Quick fix.
Comment #36
jibranComment #37
webchickThanks. /me slaps self.
Committed and pushed to 8.x.
Comment #38
dawehner/me slaps self twice
Comment #39
tstoecklerIt seems this change was in fact not backwards-compatible. I get
in Libraries API in a previously passing HEAD. I don't suggest reverting this or anything just noting here, for reference.
Comment #40
webchickShoot, yes, you are right. API freeze got a lot slushier since the OP was written. :)
Comment #41
Crell CreditAttribution: Crell commentedWe should probably write a routing-specific YAML parser that just rips code from the Symfony one. It handles both versions as well as a number of other recent improvements. (We can't use Symfony's class directly because it depends on the Symfony Config component; bah.)