yii2 icon indicating copy to clipboard operation
yii2 copied to clipboard

Fix `typecastValue` and `typecastAttributes` methods in `AttributeTypecastBehavior`

Open WinterSilence opened this issue 3 years ago • 7 comments

  • AttributeTypecastBehavior::typecastValue(): $type callback can be string
  • AttributeTypecastBehavior::typecastAttributes: typecast only active attributes
Q A
Is bugfix? ✔️
New feature?
Breaks BC? ✔️/❌
Fixed issues

WinterSilence avatar Aug 28 '22 06:08 WinterSilence

@rob006 smiles not informative, can you explain your position?

WinterSilence avatar Sep 10 '22 02:09 WinterSilence

Your PR description is laconic and does not explain what use case you're trying to fix. And I see big potential for BC breaks and footguns (is_callable() is just infinite source of problems, and it is also quite slow).

rob006 avatar Sep 10 '22 09:09 rob006

@rob006

Your PR description is laconic

it's because my English not so good :(

AttributeTypecastBehavior::typecastValue(): $type callback can be string

fixes error at using stringable callbacks

AttributeTypecastBehavior::typecastAttributes: typecast only active attributes

filters passed attributes to skip type casting unactive attributes

And I see big potential for BC breaks and footguns (is_callable() is just infinite source of problems, and it is also quite slow

is_callable() better than is_scalar(), because covers string callbacks

WinterSilence avatar Sep 10 '22 12:09 WinterSilence

filters passed attributes to skip type casting unactive attributes

Which obviously breaks BC, since you even needed to adjust unit tests.

is_callable() better than is_scalar(), because covers string callbacks

So if someone has integer() function it will be called instead of built-in typecast. If we want to support string callbacks, built-in typecasts should have precedence and call_user_func() should be used for everything else.

rob006 avatar Sep 10 '22 13:09 rob006

@rob006

Which obviously breaks BC

type casting unactive attributes is bug

since you even needed to adjust unit tests

I'm just fixed incomplete test model - active attribute must be rules()

If we want to support string callbacks, built-in typecasts should have precedence and call_user_func() should be used for everything else.

agree, fixed

WinterSilence avatar Sep 10 '22 13:09 WinterSilence

type casting unactive attributes is bug

Why? This "bug" was covered by tests.

agree, fixed

There is still problematic is_callable().

rob006 avatar Sep 10 '22 14:09 rob006

@rob006

This "bug" was covered by tests.

as you can see, changes too covered, I'm just add missed rule

There is still problematic is_callable().

what's problem?

WinterSilence avatar Sep 10 '22 15:09 WinterSilence

Closing since that's very likely result in a big BC break.

samdark avatar May 21 '23 09:05 samdark