Skip to content

Fix: Inconsistent accessor attribute name conversion#54578

Closed
pandiselvamm wants to merge 3 commits intolaravel:11.xfrom
pandiselvamm:fix/54570-inconsistent-accessor-attribute-name-conversion
Closed

Fix: Inconsistent accessor attribute name conversion#54578
pandiselvamm wants to merge 3 commits intolaravel:11.xfrom
pandiselvamm:fix/54570-inconsistent-accessor-attribute-name-conversion

Conversation

@pandiselvamm
Copy link
Copy Markdown
Contributor

This goes with issue #54570

Inconsistent accessor attribute name conversion for mutateAttributeMarkedAttribute

public function testSnakeCaseMutateAttribute()
{
$instance = new HasAttributesWithSnakeCaseConsistentCheck();
$this->assertEquals('foo2Bar', $instance->getAttribute('foo2Bar'));
Copy link
Copy Markdown
Contributor

@jackbayliss jackbayliss Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure these tests are correct, as getAttribute was working properly before the change you've made. You can test it in Tinker ie before & after.

I believe it should be testing ->append('foo_2_bar')->toArray() etc, ie the same scenario as the issue, as it's altered mutateAttributeForArray which is called by attributesToArray

(ie the test should be testing the scenario of the issue to verify it's fixed :) )

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah sure I will make a test for those too

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jackbayliss , tested with multiple scenarios including toArray() , its is working fine as excpected

@taylorotwell
Copy link
Copy Markdown
Member

I am closing this pull request because it lacks sufficient explanation, tests, or both. It is difficult for us to merge pull requests without these things because the change may introduce breaking changes to the framework.

Feel free to re-submit your change with a thorough explanation of the feature and tests - integration tests are preferred over unit tests. Please include it's benefit to end users; the reasons it does not break any existing features; how it makes building web applications easier, etc.

Thanks!

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.

4 participants