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

[5.8] Replace only last occurrence to find class name from path on event discovery #28421

Merged
merged 3 commits into from May 6, 2019

Conversation

Projects
None yet
3 participants
@paul-thebaud
Copy link
Contributor

commented May 6, 2019

This PR fix the event discovery. When having a Laravel project within /app path (it is the case with Heroku PHP apps), Laravel will replace /app twice because the str_replace is used.

My fix is to use Str::replaceLast as it will only replace the last occurrence.

@paul-thebaud paul-thebaud changed the title Replace only last occurrence to find class name from path on event discovery [5.8] Replace only last occurrence to find class name from path on event discovery May 6, 2019

@antonkomarev

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

It will be good to have tests for this case.

@paul-thebaud

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

It will be good to have tests for this case.

Since the DiscoverEvents has only static methods, and the Finder is not created using dependency injection, it is hard to test this case without Reflection for protected method.

But I don't know if it is a good idea. Any other thought?

paul-thebaud added some commits May 6, 2019

@paul-thebaud

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

As @antonkomarev suggested, I added a test for the case mentioned in this PR.

This test use Reflection to only call DiscoverEvents::classFromFile method because the Finder instance cannot be mocked because there is no dependency injection.

@taylorotwell taylorotwell merged commit 024345d into laravel:5.8 May 6, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/styleci/pr The analysis has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.