Data producer paragraph_behavior is broken with NULL default value#906
Open
mrmishmash wants to merge 4 commits intothunder:7.4.xfrom
Open
Data producer paragraph_behavior is broken with NULL default value#906mrmishmash wants to merge 4 commits intothunder:7.4.xfrom
mrmishmash wants to merge 4 commits intothunder:7.4.xfrom
Conversation
… not required, adds test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The paragraph behavior data producer as it is is broken. The default value can never be set to NULL, because if it is, the whole producer will fail and resolve to NULL, even when the behavior plugin is enabled and there is a value for the given key. Using NULL as the default value when there is no value is standard practice and as it is this cannot be done.
Throwing an exception when the plugin isn't enabled is also not ideal in my opinion. Let's say one wants to implement a behavior and fetch it on all paragraphs, even the ones that do not have it enabled, which is practical in order not to have to call it just on paragraph resolver that have it enabled, one can just enable it on a paragraph and it works if it's used in a i.e. base paragraph resolver.
Currently this isn't possible and would throw, to mitigate this, one can extend the producer and wrap the resolve() method in a try/catch block, but since paragraphs that do not have the behavior enabled will always throw, this leads to a significant performance hit because of the exception handling.
I suggest a parameter that allows the caller to determine if the producer should throw in such cases or not, with it being set to TRUE for backwards compatibility.
Make sure these boxes are checked before submitting your pull request - thank you!
If you are really awesome, then your feature is covered by additional tests. Well done!