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

Already on GitHub? Sign in to your account

make it possible to ignore route attributes in the RedirectContoller #7590

Merged
merged 3 commits into from Apr 20, 2013

Conversation

Projects
None yet
6 participants
Contributor

lsmith77 commented Apr 7, 2013

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -
_welcome:
    pattern: /{path}
    defaults:
        _controller: FrameworkBundle:Redirect:redirect
        route: licenses_projects
        ignoreAttributes: true
    requirements:
        path: .*

or

_welcome:
    pattern: /{path}
    defaults:
        _controller: FrameworkBundle:Redirect:redirect
        route: licenses_projects
        ignoreAttributes: [path]
    requirements:
        path: .*

Ensures that path isnt added as a GET parameter in case the licenses_projects doesnt have a path in the pattern.

Owner

fabpot commented Apr 20, 2013

Looks good to me. Can you add some unit tests?

Contributor

lsmith77 commented Apr 20, 2013

ok. i would like to refactor how the request is accessed to use the option to inject the request via the action signature.

Owner

fabpot commented Apr 20, 2013

You mean public function redirectAction(Request $request, $route, $permanent = false, $ignoreAttributes = false)? That would indeed be a good idea.

Contributor

lsmith77 commented Apr 20, 2013

ok .. will take care of that too. is it ok if i do this in a single PR?

Owner

fabpot commented Apr 20, 2013

sure, one PR is fine.

Contributor

lsmith77 commented Apr 20, 2013

done

fabpot added a commit that referenced this pull request Apr 20, 2013

merged branch lsmith77/redirect_controller (PR #7590)
This PR was merged into the master branch.

Discussion
----------

make it possible to ignore route attributes in the RedirectContoller

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

```
_welcome:
    pattern: /{path}
    defaults:
        _controller: FrameworkBundle:Redirect:redirect
        route: licenses_projects
        ignoreAttributes: true
    requirements:
        path: .*
```

or

```
_welcome:
    pattern: /{path}
    defaults:
        _controller: FrameworkBundle:Redirect:redirect
        route: licenses_projects
        ignoreAttributes: [path]
    requirements:
        path: .*
```

Ensures that ``path`` isnt added as a GET parameter in case the ``licenses_projects`` doesnt have a path in the pattern.

Commits
-------

190abc1 add unit tests for $ignoreAttributes
0c7b65f inject the Request via the action signature, rather than fetching it from the DIC
49062aa make it possible to ignore route attributes in the RedirectContoller

@fabpot fabpot merged commit 190abc1 into symfony:master Apr 20, 2013

1 check passed

default Scrutinizer: No Comments — Travis: Pending
Details
Contributor

mpdude commented on 0c7b65f Apr 21, 2013

Isn't that a BC break for users of this controller? Or can you still use it as described here?

Contributor

lsmith77 replied Apr 21, 2013

it can still be used as described in the docs. its a BC break for anyone that extended this controller however. not sure if this warrants an entry in the updating guide though.

Owner

fabpot replied Apr 21, 2013

@lsmith77: Adding a note in the CHANGELOG would be helpful indeed.

Contributor

lsmith77 replied Apr 21, 2013

you mean UPGRADE-2.3.md?

Owner

fabpot replied Apr 21, 2013

Just a note in the component CHANGELOG is enough.

Contributor

lsmith77 replied Apr 21, 2013

Contributor

mpdude replied Apr 21, 2013

But I have to pass the Request into the action method and that has to happen from the configured route, or am I on the wrong track?

Contributor

mpdude replied Apr 21, 2013

Wasn't aware of that. Lesson learned, thanks!

itavero commented Jun 14, 2013

@lsmith77 Is it wanted behaviour that I need to include ignoreAttributes in the ignoreAttributes array to prevent ignoreAttributes from being in the query string?
That's what happening in my dev environment right now.

Contributor

lsmith77 commented Jun 16, 2013

no .. that wouldnt make sense indeed, but i am pretty sure that when i implemented this it wasn't needed. if you are seeing this behavior now submit a test case along with optionally a fix.

Contributor

derrabus commented Jul 24, 2013

I can confirm, that I have to add ignoreAttributes to the ignoreAttributes array – at least in 2.3.2. If this is not intended behavior (and hasn't been fixed in master), I can submit a PR for that.

itavero commented Jul 24, 2013

@derrabus I totally forgot to create a new issue for that, but as stated in @lsmith77's last comment it is not wanted behavior. (in other words, I would be happy with your PR)

Contributor

derrabus commented Jul 24, 2013

Done. :-)

fabpot added a commit that referenced this pull request Jul 24, 2013

merged branch derrabus/ignore_attributes_in_redirect_controller (PR #…
…8563)

This PR was merged into the 2.3 branch.

Discussion
----------

[FrameworkBundle] RedirectCotroller: The ignoreAttributes parameter should be ignored

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

As discussed in PR #7590, when pointing a route to FrameworkBundle's `RedirectController` the attribute `ignoreAttributes` is added to the new route's attributes, if specified as array. As @lsmith77 wrote in a comment to that PR, this should not be the intended behavior. A valid workaround is to add `ignoreAttributes` itself to this array.

The problem remained undiscovered by the unit test, as `ignoreAttribues` was not included in the `_route_params` of the faked `Request` object. This PR should fix both, the unit test and the `RedirectController` itself, so the workaround mentioned above is not necessary anymore. Additionally, my fix should not break any routing configuration that is using the workaround.

Commits
-------

dc1fff0 The ignoreAttributes itself should be ignored, too.

I hope not to miss something about this PR, but as far I can see the xml schema for the routing file is not in sync, you can't define a collection of attributes to ignore, just a simple element.

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