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

get and set ignored classes #2091

Open
wants to merge 3 commits into
base: master
from

Conversation

@wickedOne
Copy link

wickedOne commented Feb 11, 2020

allow ignored classes to be set in the rest action reader

fixes #2090

allow ignored classes to be set in the rest action reader

fixes #2090
@@ -236,6 +251,26 @@ public function getParents()
return $this->parents;
}

/**
* Set ignored classes.

This comment has been minimized.

Copy link
@Tobion

Tobion Feb 12, 2020

Member

some detailed information about what this means is required

*
* @return array
*/
public function getIgnoredClasses()

This comment has been minimized.

Copy link
@Tobion

Tobion Feb 12, 2020

Member

please add return type

added return types and a little documentation for ignored classes
@wickedOne wickedOne requested a review from Tobion Feb 12, 2020
/**
* ignore several type hinted arguments.
*
* @var array

This comment has been minimized.

Copy link
@Tobion

Tobion Feb 12, 2020

Member

please remove

* These classes will be ignored for route construction when
* type hinted as method argument.
*
* @param array $ignoredClasses

This comment has been minimized.

Copy link
@Tobion

Tobion Feb 12, 2020

Member

@param string[] $ignoredClasses

/**
* Get ignored classes.
*
* @return array

This comment has been minimized.

Copy link
@Tobion

Tobion Feb 12, 2020

Member

@return string[]

*
* @return array
*/
public function getIgnoredClasses(): array

This comment has been minimized.

Copy link
@Tobion

Tobion Feb 12, 2020

Member

I don't think we need a getter for this. What's the use-case?

This comment has been minimized.

Copy link
@wickedOne

wickedOne Feb 12, 2020

Author

you might want to use it to exclude a specific class so when the default ignored classes are changed you don't have to update your code.

$reader->setIgnoredClasses(arary_diff($reader->getIgnoredClasses(), [UserInterface::class]));

This comment has been minimized.

Copy link
@xabbuh

xabbuh Feb 13, 2020

Member

What about adding addIgnoredClass()/addIgnoredClasses() instead then? That would also make the intent more clear IMO when reading the code that calls any of these methods.

This comment has been minimized.

Copy link
@wickedOne

wickedOne Feb 13, 2020

Author

for this specific use case i actually need to remove an ignored class, but i agree it makes the intent a bit clearer.
do you want me to add a addIgnoredClass and removeIgnoredClass method instead of the getter and setter?

changed array type hints to be more explicit
@wickedOne wickedOne requested a review from Tobion Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.