Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Routing] Allow force-generation of trailing parameters using eg "/exports/news.{!_format}" #29599

Merged
merged 1 commit into from Dec 24, 2018

Conversation

Projects
None yet
7 participants
@luchaninov
Copy link

commented Dec 13, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #29593
License MIT

When a route is defined as path: /exports/news.{!_format}, we should force _format be defined in defaults and the generator should generate URLs with that default when none is provided (should work with any parameter of course).

@nicolas-grekas
Copy link
Member

left a comment

awesome! just a minor comment and good on my side!

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Dec 14, 2018

Would you mind creating a doc issue/PR?

Show resolved Hide resolved src/Symfony/Component/Routing/Generator/UrlGenerator.php Outdated
$varName = substr($varName, 1);
$coalescing = true;
} else {
$coalescing = false;

This comment has been minimized.

Copy link
@stloyd

stloyd Dec 14, 2018

Contributor

Move before if so else can be removed.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Dec 14, 2018

Member

or in one line: if ($important = '!' === $varName[0]) {

This comment has been minimized.

Copy link
@luchaninov

luchaninov Dec 15, 2018

Author

@nicolas-grekas nice one. To make it more understandable added ( ):
$important = ('!' === $varName[0])

luchaninov pushed a commit to luchaninov/symfony-docs that referenced this pull request Dec 15, 2018

@luchaninov

This comment has been minimized.

Copy link
Author

commented Dec 15, 2018

Would you mind creating a doc issue/PR?

symfony/symfony-docs#10785

@OskarStark
Copy link
Contributor

left a comment

Nice improvement 👍🏻

@stloyd

stloyd approved these changes Dec 20, 2018

@nicolas-grekas nicolas-grekas force-pushed the luchaninov:master branch from c1f6d10 to 9fab3d6 Dec 24, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Dec 24, 2018

Thank you @luchaninov.

@nicolas-grekas nicolas-grekas merged commit 9fab3d6 into symfony:master Dec 24, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Dec 24, 2018

feature #29599 [Routing] Allow force-generation of trailing parameter…
…s using eg "/exports/news.{!_format}" (zavulon)

This PR was squashed before being merged into the 4.3-dev branch (closes #29599).

Discussion
----------

[Routing] Allow force-generation of trailing parameters using eg "/exports/news.{!_format}"

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #29593
| License       | MIT

When a route is defined as path: `/exports/news.{!_format}`, we should force `_format` be defined in `defaults` and the generator should generate URLs with that default when none is provided (should work with any parameter of course).

Commits
-------

9fab3d6 [Routing] Allow force-generation of trailing parameters using eg \"/exports/news.{!_format}\"
@Tobion

This comment has been minimized.

Copy link
Member

commented Dec 24, 2018

I think the solution with ! is good for forcing the generation of urls with default params. This is already something I tried to tacle before in #5180.
But I suggest to also change the matching for ! prefixed placeholders to not make them optional.
So /news.{!_format} does not match /news. Otherwise we promote duplicate URIs again. If you need /news to also match, you just create a redirect route as explained in #28633 (comment)

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Jan 2, 2019

minor #10785 [Router] Marking variable as important (zavulon)
This PR was merged into the master branch.

Discussion
----------

[Router] Marking variable as important

symfony/symfony#29599

Commits
-------

34f5dfa [Router] Marking variable as important
@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

See #29763, help wanted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.