Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#35023 closed defect (bug) (wontfix)

get_field_name() returns a bad field name if you pass it "display][avatar_size"

Reported by: pbearne's profile pbearne Owned by: johnbillion's profile johnbillion
Milestone: Priority: normal
Severity: normal Version: 4.4
Component: Widgets Keywords: has-patch has-unit-tests 2nd-opinion
Focuses: administration Cc:

Description (last modified by SergeyBiryukov)

Bug introduced in [34780] from ticket #12133.

An old hack to get this function to work with arrays was to pass a string in the form of display][avatar_size doing this to the new code return widget-foo[2][posttypes]][name as the string not widget-foo[2][posttypes][name] as expected

Attachments (3)

get_field_name.patch (1.4 KB) - added by pbearne 9 years ago.
patch and unit tests
get_field_name_alt.patch (1.2 KB) - added by pbearne 9 years ago.
I better way to fix this
get_field_name_3.patch (1.4 KB) - added by pbearne 9 years ago.
3rd patch

Download all attachments as: .zip

Change History (26)

@pbearne
9 years ago

patch and unit tests

#1 @swissspidy
9 years ago

  • Keywords has-patch has-unit-tests added

#2 @pbearne
9 years ago

This patch is not pretty but does work :-)

#3 @pbearne
9 years ago

We could add a "doing it wrong" error if felt a good idea

#4 follow-up: @johnbillion
9 years ago

This sounds like a wontfix, IMO. Passing foo][bar as the field name is fragile and I would expect that to break at some point. I'm not sure how it even worked with the previous handling of field IDs.

#5 @pbearne
9 years ago

I could change my code to pass foo[bar], but it would break any site on an old version of WP

@pbearne
9 years ago

I better way to fix this

#6 @pbearne
9 years ago

I had a think over lunch and this is better fix as it now check for the closing ] not just any [ in the string

A much better test

#7 in reply to: ↑ 4 @pbearne
9 years ago

Replying to johnbillion:

This sounds like a wontfix, IMO. Passing foo][bar as the field name is fragile and I would expect that to break at some point. I'm not sure how it even worked with the previous handling of field IDs.

I can't believe I am the only dev you was doing this.

I have pushed a patch that just changes the test to look for trailing ] if we do this I am fixed and the array strings will still work

Paul

This ticket was mentioned in Slack in #core by pbearne. View the logs.


9 years ago

#9 @joehoyle
9 years ago

I'm inclined to agree with @johnbillion, this looks like a very nasty hack in the first place, and certainly isn't part of the API. It would be nice to enable the use of arrays in fields somehow, but supporting this method of doing (which is essentially what you asking for) IMO is not a good idea. Even if we commit this fix, as it's not part of the API it's possible such a fragile think could break again in the future.

#10 follow-up: @pbearne
9 years ago

my second patch just changes the method of the test to see if a string with bra[foo][foo] is passed

The more I look at this I feel this is wrong, if we want the ability to pass an array why aren't passing a real array instead of 'sudo' array in a string ( bra[foo][foo] is not an array ) I feel tha the patch was wrong. We should be testing for is_array and switching on that not looking a "[" in string

I know this was a hack, but it worked for the last 6 years

all the function did was take a string and wrap it in brackets and prefix it doesn't now do that

Sorry to keep on but I do feel we got this wrong in 4.4 and we should fix before it is used

Paul

#11 in reply to: ↑ 10 @johnbillion
9 years ago

  • Keywords 2nd-opinion added
  • Milestone changed from Awaiting Review to 4.4.1

Replying to pbearne:

The more I look at this I feel this is wrong, if we want the ability to pass an array why aren't passing a real array instead of 'sudo' array in a string ( bra[foo][foo] is not an array ) I feel tha the patch was wrong.

The function accepts an array syntax value that is identical to what you would use in a name attribute directly. Example:

<input name="foo[bar]">
<input name="foo[bar][baz][]">

vs

$this->get_field_name( 'foo[bar]' );
$this->get_field_name( 'foo[bar][baz][]' );

Moving to the 4.4.1 milestone for consideration, regardless.

#12 @SergeyBiryukov
9 years ago

  • Description modified (diff)

#13 @welcher
9 years ago

The hack that in question was a reaction to the API not supporting the functionality introduced in [34780] - which effectively renders it no longer needed.

I understand that backwards compatibility is an issue for those implementing the work around but IMHO, I don't think we should be adding functionality to make a hack part of core. Core needs to be able to move away from hacks and workarounds when we add to or improve the API to address the instances that required them in the first place - even if that breaks the workaround ( which is kind of the point ).

Now that we have a the means of doing what the hack intended correctly, we should be triggering a __doing_it_wrong() notice at most.

#14 @pbearne
9 years ago

I hear you, but I am in a real bind I have pushed a new version of the plugin that includes a workaround. but any installs with an old version on WP 4.4 will break.

The function currently uses a test that looks string with a "[" but assumes that it closes with a closing "]"

The 3rd patch tests for the closing "]" which is safer that just testing for an opening "[" and handles "foo]" as well just incase

Last edited 9 years ago by johnbillion (previous) (diff)

@pbearne
9 years ago

3rd patch

#15 @jorbin
9 years ago

  • Owner set to johnbillion
  • Status changed from new to assigned

#16 @pbearne
9 years ago

@johnbillion if I can help please just ask - Paul

#17 @dd32
9 years ago

I'm really close to agreeing that this is a wontfix based on the fact that it was effectively hacking array support in.

Have there been any other reports of breakage due to this? If it's just the one plugin, then meh, If it's been a common thing for plugin authors to do, then I guess we should consider fixing it, but it's still very edgecasey where a plugin just switching upon $wp_version seems better.

#18 @pbearne
9 years ago

One of the reasons I have been pushing this is the feeling that the test currently used is not the best why to test to see if an "array string" is passed

I currently just looks for a "somewhere in the string (it can be the last letter). I have changed this to look for closing?" which I feel is safer as the code assumes that there is one

My Plugin may be the only one to break and all I can do is fix the string that is returned.

Luckily I had wrapped the call in a local function and I can do a replace on the "[[" with a single "and add the missing?" so I am fixed.

But I would still encourage you to change the if statement to the look for the trailing "]" as this a safer test

Paul

Last edited 9 years ago by pbearne (previous) (diff)

#19 @dd32
9 years ago

I really can't see the difference between looking for a closing vs opening bracket. If one is provided without the other then it's going to result in an invalid field anyway. Given the following strlen() call uses [ it's safer to use that same character for the tests.

In other words - We don't really care if it breaks if you pass it invalid data.

This ticket was mentioned in Slack in #core by jorbin. View the logs.


9 years ago

#21 @jorbin
9 years ago

  • Milestone changed from 4.4.1 to 4.5

The lack of decision on this means it's going to miss 4.4.1, if something is committed we can consider backporting into a potential 4.4.2

#22 @swissspidy
9 years ago

Still think that this is a wontfix as I haven't heard any other reports about this.

#23 @johnbillion
9 years ago

  • Milestone 4.5 deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.