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

Conversation

senghe
Copy link

@senghe senghe commented Nov 3, 2023

Hi all! :) I've prepared a PR with a new Form DataTransformer - it allows to cache the result to not to spam application with the same database queries. I hope you like it! 🖖

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets needed in Sylius/Sylius#15493
License MIT

@senghe senghe requested a review from a team as a code owner November 3, 2023 07:50
@senghe senghe changed the base branch from 1.11 to 1.9 November 3, 2023 07:51
/**
* @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

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 */

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

Comment on lines +81 to +85

public function clear(): void
{
self::$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
public function clear(): void
{
self::$cache = [];
}

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 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 string $identifier;

/** @var array<ResourceInterface> */
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?


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.

@loic425
Copy link
Member

loic425 commented Nov 25, 2023

Thx for your contribution,
Cause this is a new feature, it should be based on 1.11 branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants