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

[DI] [Controller] Add controller redirect response code configuration option #1101

Conversation

robfrawley
Copy link
Collaborator

Q A
Branch? 2.0
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets #970
License MIT
Doc PR included in PR

This PR re-adds the liip_imagine.controller.redirect_response_code configuration option originally merged into 1.0 (as #970) but never merged into 2.0.

Introduces a new Liip\ImagineBundle\Config\Controller\ControllerConfig service that is passed as the forth argument to Liip\ImagineBundle\Controller\ImagineController. Not passing an instance of the new ControllerConfig object will now throw a deprecation message and the ability to not do so will be removed in 3.0.

The ControllerConfig object will also reject non-redirect HTTP codes; the configured code must be one of 201, 301, 302, 303, 307, or 308 (otherwise an invalid argument exception will be thrown).


See #1011 (comment) and its following conversation for more context on the missing PRs from 2.0 that were already merged into 1.x.

@robfrawley robfrawley added Level: New Feature 🆕 This item involves the introduction of new functionality. Attn: Deprecation This issue or PR results in deprecated functionality. Attn: Minor This issue or PR is a minor problem or minor change. labels May 27, 2018
@robfrawley robfrawley added this to the 2.1.0 milestone May 27, 2018
@robfrawley robfrawley self-assigned this May 27, 2018
@robfrawley robfrawley added this to To Do in 2.1.0 Merges from Latest 1.x.x via automation May 27, 2018
Signed-off-by: Rob Frawley 2nd <rmf@src.run>
…ructor arguments are removed

Signed-off-by: Rob Frawley 2nd <rmf@src.run>
@robfrawley robfrawley moved this from To Do to In Progress in 2.1.0 Merges from Latest 1.x.x May 27, 2018
@robfrawley robfrawley moved this from In Progress to In Review in 2.1.0 Merges from Latest 1.x.x May 27, 2018
@maximgubar
Copy link
Contributor

since default branch was changed to master, please update the base branch for this PR

@dbu dbu changed the base branch from 2.0 to master August 9, 2019 08:52
Copy link
Member

@dbu dbu left a comment

Choose a reason for hiding this comment

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

looks good to me. can you please rebase so we can merge this?

@franmomu
Copy link
Contributor

franmomu commented Sep 4, 2019

This can be closed, #1220 has been merged

@dbu
Copy link
Member

dbu commented Sep 5, 2019

wrapped up in #1220

@dbu dbu closed this Sep 5, 2019
2.1.0 Merges from Latest 1.x.x automation moved this from In Review to Done Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Attn: Deprecation This issue or PR results in deprecated functionality. Attn: Minor This issue or PR is a minor problem or minor change. Level: New Feature 🆕 This item involves the introduction of new functionality.
Development

Successfully merging this pull request may close these issues.

None yet

4 participants