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

[Feat] Added support for Laravel attribute setters in Laravel preset #154

Merged
merged 7 commits into from Jun 5, 2019

Conversation

@olivernybroe
Copy link
Collaborator

commented Jun 3, 2019

Q A
Bug fix? yes
New feature? yes
Fixed tickets #135

This PR adds support for the Laravel set attribute syntax without showing it as an error.

This has been implemented by creating a custom Sniff for setters which replaces the one we used from a package. This new sniff has support for filtering methods based on a regex.

In the Laravel Preset the regex for Laravel attribute setters has been added in as methods which to ignore.

resolves: #135

@olivernybroe

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 3, 2019

@nunomaduro This PR also adds an example for testing that the Laravel Preset actually ignores attribute setters. It is not a bullet proof way, as it just checks if any errors are present in the Classes metric.

olivernybroe added some commits Jun 3, 2019

Merge branch 'master' of https://github.com/nunomaduro/phpinsights in…
…to laravel-setter

# Conflicts:
#	src/Domain/Analyser.php
@nunomaduro

This comment has been minimized.

Copy link
Owner

commented Jun 3, 2019

Sounds awesome, going to check this tonight.

@nunomaduro nunomaduro self-requested a review Jun 3, 2019

olivernybroe added some commits Jun 3, 2019

@nunomaduro

This comment has been minimized.

Copy link
Owner

commented Jun 4, 2019

@olivernybroe Thanks for this pull request. Actually I think we can do better on this: A generic feature where on the configuration you can ignore errors globally or on specific classes. What do you think?

@olivernybroe

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 5, 2019

@nunomaduro I like the idea of adding that in to ignore errors in a more broad way. However for it to work good enough to replace this sniff, it has to be specific enough to I can ignore an error based on the naming of a method with a regex.

This PR still disallows setters in models, but just ignores them if they are laravel setters. The Sniff itself just has support for ignoring methods based on a regex.

My opinion would be to first merge this in, as we are going to need custom sniff in some cases and fixing an issue like this is pretty important as a lot of users are using laravel mutators.
Then we can work on setting up a generic way to ignore classes.

Merge branch 'master' of https://github.com/nunomaduro/phpinsights in…
…to laravel-setter

# Conflicts:
#	src/Application/Console/Style.php
#	src/Domain/Analyser.php
#	src/Domain/Contracts/Repositories/FilesRepository.php
#	src/Domain/Insights/InsightCollectionFactory.php
#	src/Domain/Insights/InsightFactory.php
#	src/Domain/Insights/Sniff.php
#	src/Domain/Reflection.php
@nunomaduro

This comment has been minimized.

Copy link
Owner

commented Jun 5, 2019

@olivernybroe Alright! Can you just check why tests are not passing?

Merge branch 'master' of https://github.com/nunomaduro/phpinsights in…
…to laravel-setter

# Conflicts:
#	src/Application/Console/Style.php
#	src/Domain/Analyser.php
#	src/Domain/Contracts/Repositories/FilesRepository.php
#	src/Domain/Insights/InsightCollectionFactory.php
#	src/Domain/Insights/InsightFactory.php
#	src/Domain/Insights/Sniff.php
#	src/Domain/Reflection.php

@nunomaduro nunomaduro merged commit 40e8641 into nunomaduro:master Jun 5, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nunomaduro

This comment has been minimized.

Copy link
Owner

commented Jun 5, 2019

Very good job! Thanks for this @olivernybroe. Fell free to perform the next release: https://github.com/nunomaduro/phpinsights/blob/master/RELEASE.md. Please ask me if you have any questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.