Skip to content

Conversation

hgraca
Copy link
Contributor

@hgraca hgraca commented Nov 7, 2017

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Documented? yes
Fixed tickets
License MIT

We would very much like to have the possibility of adding an exception to overblog_graphql.definitions.exceptions.errors and have its subclasses also be caught and handled by the ErrorHandler. (it would avoid us to explicitly add all exceptions to the config)

This PR adds an option to the config, to do exactly that.

@hgraca hgraca force-pushed the add_inheritance_exception_mapping branch 3 times, most recently from 6158213 to b3347f8 Compare November 7, 2017 20:37
@renatomefi
Copy link
Contributor

I don't get exactly what's happening.
I'd expect to any child or exception implementing a certain interface to be caught properly, is that the case? If not please give me a better overview

@hgraca
Copy link
Contributor Author

hgraca commented Nov 9, 2017

In the current implementation it matches the on the exception fqcn that we put in the config and on the caught exception fqcn, so it is string comparison, it does not take inheritance into consideration.
This PR adds an option to check inheritance as well, when the exact exception is not set in the config.

@renatomefi
Copy link
Contributor

renatomefi commented Nov 9, 2017

@hgraca can we try to make it more php native (maybe using instance of or so) then instead of keeping the string comparison?

I'd even consider not making it optional, since it's a php behavior to treat exceptions with inheritance.

@hgraca
Copy link
Contributor Author

hgraca commented Nov 9, 2017

Well, the code I added does exactly that, it tries to find a match using instanceof.

However, if we replace the existing string comparison, we will be changing expected behaviour which means it is a BC break, which means a major version bump, etc...
Im fine either way, but I think that was why @mcg-web asked me to make an option for this.

Furthermore, the string comparison is much faster (it actually checks if the fqcn exists as an array key), so if it is already there, then we don't need to loop over the whole list of exceptions and to instanceof until we find a match.

@renatomefi
Copy link
Contributor

A way to make it faster is to use reflection, gather the class/interfaces/parents and then make an array intersection, but might be a bit overkill.
I think it's a good break change, since it's going accordingly to php standards and master is already pointing to 0.10. @mcg-web would you consider making this default?
The code will look cleaner as well

@hgraca hgraca force-pushed the add_inheritance_exception_mapping branch from b3347f8 to 44bea52 Compare November 9, 2017 15:21
@mcg-web
Copy link
Contributor

mcg-web commented Nov 9, 2017

@renatomefi this is a sensitive change, this can result on security issues. I don't want to expose critical errors to users because of this, that's why I prefer leave the choice to developer to enable or not this feature in a first place. I agree error mapping need to be refactor... Next step will be to use native GraphQL PHP error handler but since the bundle implements this feature before the lib, we must not break compatibility.

@barduck007 barduck007 merged commit 634f154 into overblog:master Nov 10, 2017
@hgraca
Copy link
Contributor Author

hgraca commented Nov 10, 2017

🎉
tkx guys!!

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