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

Take Hash with options inside Array in #url_for #9599

Merged
merged 1 commit into from Nov 15, 2013

Conversation

Projects
None yet
9 participants
@ognevsky

ognevsky commented Mar 7, 2013

Allows you to code like this:

= link_to 'Edit', [:edit, @post, { author_id: current_user.id }]
@ognevsky

This comment has been minimized.

Show comment
Hide comment
@ognevsky

ognevsky commented Mar 7, 2013

cc @drogus

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Mar 7, 2013

Member

Needs tests and a changelog entry.

Member

carlosantoniodasilva commented Mar 7, 2013

Needs tests and a changelog entry.

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Mar 8, 2013

Member

great patch 👍 looking forward to it!

Member

senny commented Mar 8, 2013

great patch 👍 looking forward to it!

@senny

This comment has been minimized.

Show comment
Hide comment
@senny
Member

senny commented Mar 8, 2013

@bjoernbur

This comment has been minimized.

Show comment
Hide comment
@bjoernbur

bjoernbur Mar 8, 2013

thanks for this patch...

bjoernbur commented Mar 8, 2013

thanks for this patch...

@ognevsky

This comment has been minimized.

Show comment
Hide comment
@ognevsky

ognevsky Mar 8, 2013

I'm done with tests and a changelog entry.

cc @carlosantoniodasilva @senny

ognevsky commented Mar 8, 2013

I'm done with tests and a changelog entry.

cc @carlosantoniodasilva @senny

@carlosantoniodasilva

View changes

Show outdated Hide outdated actionpack/test/controller/url_for_test.rb
@carlosantoniodasilva

View changes

Show outdated Hide outdated actionpack/CHANGELOG.md
@ognevsky

This comment has been minimized.

Show comment
Hide comment
@ognevsky

ognevsky commented Mar 8, 2013

@egilburg

This comment has been minimized.

Show comment
Hide comment
@egilburg

egilburg Mar 8, 2013

Contributor

+1 for the behaviour, but why extract_options! is needed for the Array case but not for the basic else case? Could perhaps the fix better live inside the polymorphic_path/polymorphic_url methods themselves?

Contributor

egilburg commented Mar 8, 2013

+1 for the behaviour, but why extract_options! is needed for the Array case but not for the basic else case? Could perhaps the fix better live inside the polymorphic_path/polymorphic_url methods themselves?

@ognevsky

This comment has been minimized.

Show comment
Hide comment
@ognevsky

ognevsky Mar 8, 2013

@egilburg there are a lot of else cases, not only arrays. So, it would break a lot of tests and code (actually, I tried it in my first attempt).

Look, polymorphic helpers already have this functionality, you can path url options as the second param (as I do with extract_options!).

You can use it like polymorphic_path [:some, :path, :here], param: 'value', the problem is that you can't set this param without polymorphic_path invocation, so you can't actually write link_to post.title, [:edit, :admin, @post], param: 'value'.

I hope I've cleared this.

ognevsky commented Mar 8, 2013

@egilburg there are a lot of else cases, not only arrays. So, it would break a lot of tests and code (actually, I tried it in my first attempt).

Look, polymorphic helpers already have this functionality, you can path url options as the second param (as I do with extract_options!).

You can use it like polymorphic_path [:some, :path, :here], param: 'value', the problem is that you can't set this param without polymorphic_path invocation, so you can't actually write link_to post.title, [:edit, :admin, @post], param: 'value'.

I hope I've cleared this.

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Mar 8, 2013

Member

@egilburg doing it in polymorphic_path was tried in #7259 and rejected because of clashes with form_for behaviour.

@josevalim are you still 👎 on this behavior ?

Member

pixeltrix commented Mar 8, 2013

@egilburg doing it in polymorphic_path was tried in #7259 and rejected because of clashes with form_for behaviour.

@josevalim are you still 👎 on this behavior ?

@ognevsky ognevsky referenced this pull request May 3, 2013

Closed

Delegate each to all #9635

@senny

This comment has been minimized.

Show comment
Hide comment
@senny
Member

senny commented Jun 15, 2013

@josevalim ping

@ognevsky

This comment has been minimized.

Show comment
Hide comment
@ognevsky

ognevsky commented Oct 16, 2013

@senny ping

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Oct 16, 2013

Member

@ognevsky we need to wait for @josevalim to jump in.

Member

senny commented Oct 16, 2013

@ognevsky we need to wait for @josevalim to jump in.

@ognevsky

This comment has been minimized.

Show comment
Hide comment
@ognevsky

ognevsky Oct 16, 2013

@senny ok, let's wait.

ognevsky commented Oct 16, 2013

@senny ok, let's wait.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Oct 16, 2013

Contributor

The issue here is what is going to be considered the form object in form_for if the last element is a hash. Can a hash be a form object today? If so, how this patch affect form_for? If we have those semantics clear, I don't see any problem with this patch.

Contributor

josevalim commented Oct 16, 2013

The issue here is what is going to be considered the form object in form_for if the last element is a hash. Can a hash be a form object today? If so, how this patch affect form_for? If we have those semantics clear, I don't see any problem with this patch.

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Oct 16, 2013

Member

@ognevsky if you can add tests for form_for with this behaviour and rebase so it'll merge cleanly then I think we have a 👍

Member

pixeltrix commented Oct 16, 2013

@ognevsky if you can add tests for form_for with this behaviour and rebase so it'll merge cleanly then I think we have a 👍

@ognevsky

This comment has been minimized.

Show comment
Hide comment
@ognevsky

ognevsky Nov 12, 2013

@josevalim @pixeltrix sorry for the long wait.

Jose, this patch doesn't affect to form_for, because it uses polymorphic_path here: https://github.com/rails/rails/blob/master/actionview/lib/action_view/helpers/form_helper.rb#L453, not the url_for.

So, I don't change form_for behavior.

ognevsky commented Nov 12, 2013

@josevalim @pixeltrix sorry for the long wait.

Jose, this patch doesn't affect to form_for, because it uses polymorphic_path here: https://github.com/rails/rails/blob/master/actionview/lib/action_view/helpers/form_helper.rb#L453, not the url_for.

So, I don't change form_for behavior.

@ognevsky

This comment has been minimized.

Show comment
Hide comment
@ognevsky

ognevsky Nov 12, 2013

@josevalim @pixeltrix I've rebased and updated this request.
I'll add a record to changelog before the merge, it changes very often.

ognevsky commented Nov 12, 2013

@josevalim @pixeltrix I've rebased and updated this request.
I'll add a record to changelog before the merge, it changes very often.

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode
Member

chancancode commented Nov 12, 2013

@ognevsky

This comment has been minimized.

Show comment
Hide comment
@ognevsky

ognevsky Nov 12, 2013

@chancancode oops, I'll fix it in the morning

ognevsky commented Nov 12, 2013

@chancancode oops, I'll fix it in the morning

@ognevsky

This comment has been minimized.

Show comment
Hide comment
@ognevsky

ognevsky Nov 12, 2013

Fixed. I've added this failing spec to the commit accidentally, it should never pass (because form_for doesn't have this features)

ognevsky commented Nov 12, 2013

Fixed. I've added this failing spec to the commit accidentally, it should never pass (because form_for doesn't have this features)

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Nov 15, 2013

Member

@ognevsky can you add a CHANGELOG entry and squash your commits - then I'll merge it.

Member

pixeltrix commented Nov 15, 2013

@ognevsky can you add a CHANGELOG entry and squash your commits - then I'll merge it.

@ognevsky

This comment has been minimized.

Show comment
Hide comment
@ognevsky

ognevsky commented Nov 15, 2013

@pixeltrix I'm ready

pixeltrix added a commit that referenced this pull request Nov 15, 2013

Merge pull request #9599 from ognevsky/hash-inside-array-in-url-for
Take Hash with options inside Array in #url_for

@pixeltrix pixeltrix merged commit ce06d57 into rails:master Nov 15, 2013

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix
Member

pixeltrix commented Nov 15, 2013

@ognevsky thanks!

@nathanbertram

This comment has been minimized.

Show comment
Hide comment
@nathanbertram

nathanbertram commented Feb 21, 2014

You da man!

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