IBX-11826: Cleaned up http-cache ResponseTagger deprecations - #82
Conversation
…ated tests to mock `ResponseTagger` instead of specific tagger classes.
14d267e to
21f8d30
Compare
| '%s does not implement supports(). This will be required in 6.0, supports() will be a part of ResponseTagger interface', | ||
| get_debug_type($tagger), | ||
| ); | ||
| if ($tagger->supports($value)) { |
There was a problem hiding this comment.
Going back the initial idea that pushed us towards these changes - maybe we throw an exception* if there's no tagger supporting given value? As this must be a mistake.
The origial report said that if you pass a Content object (instead of ContentInfo) to be tagged, nothing happens and you get no information that something is wrong.
*or log a warning, or log a warning in dev mode, or something else
There was a problem hiding this comment.
I went with your "log a warning" idea, but split it by environment:
debug mode (dev/test) → throw an InvalidArgumentException, so the mistake surfaces immediately, including in the test suite
prod → log a warning instead
There was a problem hiding this comment.
Looks good to me at least.
There was a problem hiding this comment.
Looks great to me, thank you! 🙇
|
7 participants
Related PRs:
#78
Description:
This PR finalizes the
ResponseTaggercleanup introduced in ibexa/http-cache#78.It adds
supports(mixed $value): boolto theResponseTaggerinterface, removesAbstractValueTagger, and simplifiesDispatcherTaggerby dropping the BC fallback for implementations withoutsupports().Additionally,
DispatcherTaggernow signals when no tagger supports the given value — throwing anInvalidArgumentExceptionin debug mode and logging a warning otherwise — instead of silently doing nothing.###Regressions:
ibexa/commerce#1841
###QA
Need some sanities.