Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
nicolas-grekas merged 1 commit intosymfony:masterfromluchaninov:master
Dec 24, 2018

Conversation

@luchaninov
Copy link
Contributor

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#29593
LicenseMIT

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

ustal, TheBurningRed, TomasVotruba, vudaltsov, and Kocal reacted with thumbs up emojiro0NL, andreybolonin, vudaltsov, and Kocal reacted with hooray emoji
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

@nicolas-grekas
Copy link
Member

Would you mind creating a doc issue/PR?

$varName =substr($varName,1);
$coalescing =true;
}else {
$coalescing =false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Move beforeif soelse can be removed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

@luchaninov
Copy link
ContributorAuthor

Would you mind creating a doc issue/PR?

symfony/symfony-docs#10785

Copy link
Contributor

@OskarStarkOskarStark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Nice improvement 👍🏻

@nicolas-grekas
Copy link
Member

Thank you@luchaninov.

luchaninov, andreybolonin, and HeahDude reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas merged commit9fab3d6 intosymfony:masterDec 24, 2018
nicolas-grekas added a commit that referenced this pull requestDec 24, 2018
…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       | MITWhen 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
Copy link
Contributor

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)

nicolas-grekas and HeahDude reacted with thumbs up emoji

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull requestJan 2, 2019
This PR was merged into the master branch.Discussion----------[Router] Marking variable as importantsymfony/symfony#29599Commits-------34f5dfa [Router] Marking variable as important
@nicolas-grekas
Copy link
Member

See#29763, help wanted.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@OskarStarkOskarStarkOskarStark approved these changes

+1 more reviewer

@stloydstloydstloyd approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

6 participants

@luchaninov@nicolas-grekas@Tobion@stloyd@OskarStark@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp