search

Getbeans / Beans

Beans WordPress Theme Framework. The default branch is set to development, please switch to the master branch for production.
https://www.getbeans.io
Other
392 stars 61 forks source link

restore v1.4 behavior for beans_replace_attribute() #331

Closed mjauvin closed 5 years ago

mjauvin commented 6 years ago

In Beans v1.4, it was possible to call beans_replace_attribute with an empty "new_value" which replaced all values of the attribute with "value".

This behavior was broken in version 1.5.x

christophherr commented 6 years ago

Hey @mjauvin,

Thank you for contributing to Beans!

This was a deliberate adjustment in https://github.com/Getbeans/Beans/pull/114#issue-168938455

Improved beans_replace_attribute: Removed confusing code that let "value" behave like "new_value". Always use the "new_value" for overwriting the current value. Added the ability to replace ALL of the values. How? Set the "value" argument to an empty (false, empty string, or null).

On a side note, one of our tests is failing with your changes.

mjauvin commented 6 years ago

That requires modifying ALL calls to this function on ALL websites to ADD a value, this is pretty bad.

I thought Beans made a point on NOT making any breaking changes between releases, didn't it?

On Fri, Aug 31, 2018 at 12:04 PM Christoph Herr notifications@github.com wrote:

Hey @mjauvin https://github.com/mjauvin,

Thank you for contributing to Beans!

This was a deliberate adjustment in #114 (comment) https://github.com/Getbeans/Beans/pull/114#issue-168938455

Improved beans_replace_attribute: Removed confusing code that let "value" behave like "new_value". Always use the "new_value" for overwriting the current value. Added the ability to replace ALL of the values. How? Set the "value" argument to an empty (false, empty string, or null).

On a side note, one of our tests is failing with your changes.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Getbeans/Beans/pull/331#issuecomment-417711486, or mute the thread https://github.com/notifications/unsubscribe-auth/AB65vj8l97H64ny7oGwTyx7F-DSY89P-ks5uWV6WgaJpZM4WVY_6 .

-- Marc

christophherr commented 6 years ago

@mjauvin I took the liberty of fixing the tests and switching the logic of your PR. Requesting reviews because I added code in this PR