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

Add a failing test for a URL helper that was broken by a6b9ea2. #14684

Conversation

jcoglan
Copy link
Contributor

@jcoglan jcoglan commented Apr 10, 2014

Between Rails 4.0.3 and 4.0.4 (and into 4.1.0), the routes in this test stopped producing the URL helper comment_path -- it became blog_comment_path instead. This was fixed in a6b9ea2 but the patch breaks the expected new_blog_post_comment_path helper, which was working until that patch landed.

@rafaelfranca
Copy link
Member

cc @pixeltrix

@rafaelfranca rafaelfranca added this to the 4.0.5 milestone Apr 10, 2014
pixeltrix added a commit that referenced this pull request Apr 11, 2014
…-helper-bug

Add a failing test for a URL helper that was broken by a6b9ea2.

(cherry picked from commit 1424482)

Conflicts:
	actionpack/CHANGELOG.md
pixeltrix added a commit that referenced this pull request Apr 11, 2014
…-helper-bug

Add a failing test for a URL helper that was broken by a6b9ea2.

(cherry picked from commit 1424482)

Conflicts:
	actionpack/CHANGELOG.md
@pixeltrix pixeltrix closed this in e10f26f Apr 11, 2014
pixeltrix added a commit that referenced this pull request Apr 11, 2014
…-helper-bug

Add a failing test for a URL helper that was broken by a6b9ea2.
pixeltrix added a commit that referenced this pull request May 21, 2014
Since `:shallow` may be set at any point in the resource nesting we should
only make the new and collection routes shallow when the parent is shallow.

This is a bit of a hack but until the mapper is refactored to an object graph
instead of a hash of merged values it's the best we can do.

Fixes #14684.

(cherry picked from commit e10f26f)

Conflicts:
	actionpack/CHANGELOG.md
@leods92
Copy link

leods92 commented Aug 20, 2014

This requires a first level resource (i.e. not nested) to set :shallow to true.
IMO that doesn't make sense; shouldn't shallow only be used with nested resources?
I hadn't had problems with nesting and shallow until this was introduced this "fix" in 4.0.6.
I wonder if there are more applications that broke with this.

Is setting :shallow to first-level resources really expected?

Thanks! :)

@pixeltrix
Copy link
Contributor

@leods92 :shallow is a scope option so it can be set via the :shallow option to scope, resource & resources or by using the shallow helper. Whilst it isn't applied to the first level resource it is applied to any nested resources.

@leods92
Copy link

leods92 commented Aug 21, 2014

@pixeltrix Just to be clear: the expected behaviour is to set :shallow to first-level resources even though they won't change at all, isn't it? Not only should one set :shallow in a nested resource but also in its parent/first-level resource. If that's the expected behaviour, be aware that this commit breaks applications that had been working since version 3, it broke mine...

@pixeltrix
Copy link
Contributor

@leods92 I've merged some fixes to shallow routes that haven't been released yet - can you test against the 4.1.6.rc1 release and if there's a regression from a previous release please file an issue.

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

Successfully merging this pull request may close these issues.

None yet

4 participants