EZP-31707: Fixed crashes when ezcontentquery field parameters are invalid #48
Conversation
|
Could you rebase with 2.2 branch, we are going to release 2.2 for Ibexa Platform 3.2 |
| new QueryResultsPagerFantaAdapter( | ||
| $this->queryFieldService, $content, $fieldDefinitionIdentifier | ||
| ) | ||
| $adapter = new QueryResultsPagerFantaAdapter( |
There was a problem hiding this comment.
Maybe it could be better to close it in some factory, what do you think?
There was a problem hiding this comment.
I'm not so sure about that.
Introducing a factory here means increasing the complexity of this particular code, expecting different PagerFantaAdapters to be introduced in some cases. Currently added code simply decorates the existing adapter if debug property is set to true.
It would also not get rid of the debug property, since the second, pagination-less variant just a few lines below would also still be required. This would mean it would cause the mentioned factory to be also responsible for that second variant / another service be introduced / debug property be left as-is in the injector.
All-in-all, I don't really see the benefit of introducing a factory now. If new adapter variants show up, then I'd be up for it.
There was a problem hiding this comment.
It is not as complex code that factory is must-have here but I see some advantages of that approach. It may increase readability - but it is my subjective opinion ;) however similar effect you can achieve by extracting it to a private function.
I not sure about increasing complexity here. if it really happened, it wouldn't be a problem because current complexity is rather low. However, in my opinion, it should reduce complexity because some logic will be moved out of the class and it should be noticeable when you would decide to write a unit test for that.
but probably it is not a game-changer here ;) and this discussion going to have more line than code ;) it was just proposal
There was a problem hiding this comment.
Yes, testing where having a factory with an easily describable behavior will make a difference.
There was a problem hiding this comment.
@kaff @bdunogier code has been redesigned. Instead of changing the behavior of QueryResultsInjector and wrapping PagerFantaAdapter with a decorator, a production-only service (based on kernel.debug parameter) is added to service container that decorates QueryFieldService service.
|
@lserwatka PR has been rebased to package version 2.2. However, it is compatible with earlier versions and can be rebased further if you wish. |
|
@kaff I'm not familiar with spec tests used in this package, give me a second to see. |
|
@kaff please see adc9225 for provided tests.
Now that I look at it I think I'll change the approach and wrap calls to |
|
You can merge it up to 2.2 and master. |
Steveb-p
changed the title
v3.1This PR prevents pages with invalid
ezcontentquerycontent from crashing. On production environments pages will receive empty results instead. In development, exceptions will be thrown as it happens currently without change.Introduces
psr/logdependency to allow normal logging to pick up invalid configuration of fields in production as much as possible.