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

Fix typecastValue and typecastAttributes methods in AttributeTypecastBehavior #19528

Closed
wants to merge 11 commits into from

Conversation

WinterSilence
Copy link
Contributor

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

…ecastBehavior`

- `AttributeTypecastBehavior::typecastValue()`: `$type` callback can be string
- `AttributeTypecastBehavior::typecastAttributes`: typecast only active attributes
@WinterSilence WinterSilence marked this pull request as ready for review August 28, 2022 08:39
@WinterSilence
Copy link
Contributor Author

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

@rob006
Copy link
Contributor

rob006 commented Sep 10, 2022

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).

@WinterSilence
Copy link
Contributor Author

@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

@rob006
Copy link
Contributor

rob006 commented Sep 10, 2022

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
> If we want to support string callbacks, built-in typecasts should have precedence and call_user_func() should be used for everything else.
@WinterSilence
Copy link
Contributor Author

@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

@rob006
Copy link
Contributor

rob006 commented Sep 10, 2022

type casting unactive attributes is bug

Why? This "bug" was covered by tests.

agree, fixed

There is still problematic is_callable().

@WinterSilence
Copy link
Contributor Author

@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?

@samdark samdark added this to the 2.0.48 milestone Sep 16, 2022
@samdark samdark requested a review from a team September 16, 2022 07:11
@WinterSilence WinterSilence requested review from schmunk42, rob006 and samdark and removed request for samdark, schmunk42 and rob006 September 27, 2022 17:50
@samdark
Copy link
Member

samdark commented May 21, 2023

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

@samdark samdark closed this May 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants