Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ResourceToIdentifierCacheableTransformer form data transformer #771

Open
wants to merge 6 commits into
base: 1.9
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
<?php

/*
* This file is part of the Sylius package.
*
* (c) Paweł Jędrzejewski
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

declare(strict_types=1);

namespace Sylius\Bundle\ResourceBundle\Form\DataTransformer;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
namespace Sylius\Bundle\ResourceBundle\Form\DataTransformer;
namespace Sylius\Resource\Symfony\Form\DataTransformer;

So it should be placed into src/Component/src/Symfony/Form/DataTransformer directory.


use Sylius\Component\Resource\Model\ResourceInterface;
use Sylius\Component\Resource\Repository\RepositoryInterface;
use Symfony\Component\Form\DataTransformerInterface;
use Symfony\Component\Form\Exception\TransformationFailedException;
use Symfony\Component\PropertyAccess\PropertyAccess;
use Webmozart\Assert\Assert;

final class ResourceToIdentifierCacheableTransformer implements DataTransformerInterface
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
final class ResourceToIdentifierCacheableTransformer implements DataTransformerInterface
final class CachedResourceToIdentifierTransformer implements DataTransformerInterface

Though that's just a matter of taste

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested name is in line with the rest of the sylius, I vote for it as well

{
private RepositoryInterface $repository;

private string $identifier;

/** @var array<ResourceInterface> */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** @var array<ResourceInterface> */
/** @var array<int|string, ResourceInterface> */

Think it should work

private static array $cache;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private static array $cache;
private array $cache;

Pretty sure making it static could potentially leak objects between different instances.
To leave it as static you'd need to prefix the key w/ a class name, make it a nested array or something like that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible or needed to make use of psr CacheInterface/symfony cache for this?


public function __construct(RepositoryInterface $repository, ?string $identifier = null)
{
$this->repository = $repository;
$this->identifier = $identifier ?? 'id';
}

/**
* @psalm-suppress MissingParamType
*
* @param mixed $value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param mixed $value
* @param object|null $value

*/
public function transform($value)
{
if (null === $value) {
return null;
}

/** @psalm-suppress ArgumentTypeCoercion */
Assert::isInstanceOf($value, $this->repository->getClassName());

return PropertyAccess::createPropertyAccessor()->getValue($value, $this->identifier);
}

/** @param mixed $value */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** @param mixed $value */
/** @param int|string|null $value */

public function reverseTransform($value): ?ResourceInterface
{
if (null === $value) {
return null;
}

if (isset(self::$cache[$value])) {
return self::$cache[$value];
}

/** @var ResourceInterface|null $resource */
$resource = $this->repository->findOneBy([$this->identifier => $value]);
if (null === $resource) {
throw new TransformationFailedException(sprintf(
'Object "%s" with identifier "%s"="%s" does not exist.',
$this->repository->getClassName(),
$this->identifier,
$value,
));
}

self::$cache[$value] = $resource;

return $resource;
}

public function clear(): void
{
self::$cache = [];
}
Comment on lines +81 to +85
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public function clear(): void
{
self::$cache = [];
}

}
10 changes: 8 additions & 2 deletions src/Bundle/Storage/CookieStorage.php
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,13 @@ public static function getSubscribedEvents(): array

public function onKernelRequest(RequestEvent $event): void
{
$isMainRequest = false;

/** @phpstan-ignore-next-line */
if (\method_exists($event, 'isMainRequest')) {
$isMainRequest = $event->isMainRequest();
} else {
/** @phpstan-ignore-next-line */
} elseif (\method_exists($event, 'isMasterRequest')) {
$isMainRequest = $event->isMasterRequest();
}
if (!$isMainRequest) {
Expand All @@ -60,10 +63,13 @@ public function onKernelRequest(RequestEvent $event): void

public function onKernelResponse(ResponseEvent $event): void
{
$isMainRequest = false;

/** @phpstan-ignore-next-line */
if (\method_exists($event, 'isMainRequest')) {
$isMainRequest = $event->isMainRequest();
} else {
/** @phpstan-ignore-next-line */
} elseif (\method_exists($event, 'isMasterRequest')) {
$isMainRequest = $event->isMasterRequest();
}
if (!$isMainRequest) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
<?php

/*
* This file is part of the Sylius package.
*
* (c) Paweł Jędrzejewski
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

declare(strict_types=1);

namespace spec\Sylius\Bundle\ResourceBundle\Form\DataTransformer;

use PhpSpec\ObjectBehavior;
use Prophecy\Argument;
use Sylius\Component\Resource\Model\ResourceInterface;
use Sylius\Component\Resource\Repository\RepositoryInterface;
use Symfony\Component\Form\Exception\TransformationFailedException;

final class ResourceToIdentifierCacheableTransformerSpec extends ObjectBehavior
{
function let(RepositoryInterface $repository): void
{
$this->beConstructedWith($repository, 'id');
}

function it_does_not_reverses_null_value(RepositoryInterface $repository): void
{
$repository->findOneBy(Argument::any())->shouldNotBeCalled();

$this->reverseTransform(null)->shouldReturn(null);
}

function it_throws_an_exception_on_non_existing_resource(RepositoryInterface $repository): void
{
$repository->getClassName()->willReturn(ResourceInterface::class);
$repository->findOneBy(['id' => 6])->willReturn(null);

$this->shouldThrow(TransformationFailedException::class)->during('reverseTransform', [6]);
}

function it_reverse_transform_identifier_to_resource(RepositoryInterface $repository, ResourceInterface $resource): void
{
$repository->findOneBy(['id' => 5])->willReturn($resource);

$this->reverseTransform(5)->shouldReturn($resource);
}

function it_reverse_transforms_identifier_to_resource_using_cache(
RepositoryInterface $repository,
ResourceInterface $resource,
): void {
$this->clear();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a method just for the specs to work is a bad idea. Even more so since it's not in any interface. Normally for something like this, you could use ResetInterface, but in this particular case, it's useless since this is not a service living in the container and instead gets recreated all the time, it'd be confusing.

Remove the ::clear, and as for the test itself, you can use reflection to set the value of cache


$repository->findOneBy(['id' => 5])
->willReturn($resource)
->shouldBeCalledOnce()
;

$this->reverseTransform(5)->shouldReturn($resource);
$this->reverseTransform(5)->shouldReturn($resource);
}

function it_transforms_null_value_to_empty_string(RepositoryInterface $repository): void
{
$repository->getClassName()->willReturn(ResourceInterface::class);

$this->transform(null)->shouldReturn(null);
}

function it_transforms_resource_in_identifier(RepositoryInterface $repository, ResourceInterface $resource): void
{
$repository->getClassName()->willReturn(ResourceInterface::class);

$resource->getId()->willReturn(6);

$this->transform($resource)->shouldReturn(6);
}
}
Loading