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

[8.x] Added a method to the Macroable trait that removes all configured macros. #39633

Merged
merged 3 commits into from
Nov 16, 2021
Merged

[8.x] Added a method to the Macroable trait that removes all configured macros. #39633

merged 3 commits into from
Nov 16, 2021

Conversation

mad-briller
Copy link
Contributor

This pull request adds a static flushMacros method to the Macroable trait that removes all the configured macros.

The primary use of this method is in a unit testing environment. When running multiple test cases that add static macros in the same process, there can be side-effects between test cases caused by the use of static members.

When combined with either the --random-order flag to PHPUnit or parallelized testing, these side effects can cause intermittent failures, as tests may or may not pass depending on the order / process they are ran in and the result of the side effects.
Ideally each unit test case is ran in a sanitized environment to ensure the developer is testing exactly what they intended to test, and aren't relying on any aspects setup outside of the current test case.

I was unsure as to whether the framework should enforce the running of flushMacros on contained Macroable classes, or whether the developer should handle this manually so i haven't done that.
Also enforcing that macros are always flushed is a not fully backwards-compatible change, whereas adding a method to allow developers to handle it themselves is.

Thanks for your time.

Copy link
Member

@nunomaduro nunomaduro left a comment

Choose a reason for hiding this comment

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

This pull request looks good, and useful, to me.

src/Illuminate/Macroable/Traits/Macroable.php Outdated Show resolved Hide resolved
@mad-briller mad-briller changed the title Added a method to the Macroable trait that removes all configured macros. [8.x] Added a method to the Macroable trait that removes all configured macros. Nov 16, 2021
@mad-briller
Copy link
Contributor Author

rebased the branch because i noticed i had my work email set in git and not my noreply github one, whoops!

@taylorotwell taylorotwell merged commit b2669a8 into laravel:8.x Nov 16, 2021
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

3 participants