EZP-31810: Compound SiteAccess matcher will include serialized 'pathinfo' and 'queryParams' attributes unnecessarily #3061
Conversation
|
LGTM, but should be targeting |
RegexURINormalizer, URITextNormalizer
adamwojs
requested review from
alongosz,
kaff,
mikadamczyk,
Nattfarinn,
ViniTou and
webhdx
and removed request for
a team
| */ | ||
| declare(strict_types=1); | ||
|
|
||
| namespace eZ\Publish\Core\MVC\Symfony\Component\Tests\Serializer\Stubs; |
There was a problem hiding this comment.
In my opinion, it looks more like Fake than Stub but I don't know that we want to stick strictly to definition ;)
| */ | ||
| declare(strict_types=1); | ||
|
|
||
| namespace eZ\Publish\Core\MVC\Symfony\Component\Tests\Serializer\Stubs; |
There was a problem hiding this comment.
In my opinion, it looks more like Fake than Stub but I don't know that we want to stick strictly to definition ;)
| */ | ||
| declare(strict_types=1); | ||
|
|
||
| namespace eZ\Publish\Core\MVC\Symfony\Component\Tests\Serializer\Stubs; |
There was a problem hiding this comment.
In my opinion, it looks more like Fake than Stub but I don't know that we want to stick strictly to definition ;)
| $data = parent::normalize($object, $format, $context); | ||
| foreach (array_keys($data) as $property) { | ||
| if (!in_array($property, $this->getAllowedProperties())) { | ||
| unset($data[$property]); |
There was a problem hiding this comment.
I'm wondering that this logic is enough tested. I know that testing abstract class is often subject of long discussions ;) but..
as we see this part of code turned out essential for our client.
Deleting unnecessary keys feature was covered by testing classes extending abstract and I have a feeling that this important part of code is tested a bit "by accident". For now, we have in the system some class which has some properties which shouldn't be serialized - but it can change in the future. My point is that maybe it could be a good idea to test it explicitly ;)
There was a problem hiding this comment.
Tested various SiteAccess matcher combinations taking into account the scenario described in the ticket (Varnish making too many requests because of too much detail included in serialized SiteAccess), but also additional scenarios involving permissions and other parts of the system (e.g. Page Builder) to make sure that the fix works correctly.
It fixes the issue and I haven't found anything wrong with it, QA approved.
I had some issues with URIText matcher and Page Builder (unrelated to this PR) that I will investigate tomorrow and report to JIRA if needed.
|
@adamwojs Could you merge it up? |
|
@lserwatka done. |
…nfo' and 'queryParams' attributes unnecessarily (#3061) * EZP-31810: Aligned Normalizer implementation for Compound matcher with __sleep method * EZP-31810: Impl. Normalizer for HostElement, Map, Regex and URIElement matchers * EZP-31810: Updated \eZ\Publish\Core\MVC\Symfony\Component\Serializer\SerializerTrait * EZP-31810: Fixed unit tests * EZP-31810: Added unit tests for HostTextNormalizer, RegexHostNormalizer, RegexURINormalizer, URITextNormalizer (cherry picked from commit 5d0d02a)
|
Cherry-picked to 6.13 in d86c763 |
7.xTODO:
$ composer fix-cs).