10000 [Form] Added support for caching choice lists based on options by HeahDude · Pull Request #30994 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Form] Added support for caching choice lists based on options #30994

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

Merged
merged 1 commit into from
Feb 21, 2020
Merged
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
93 changes: 42 additions & 51 deletions src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
use Symfony\Bridge\Doctrine\Form\DataTransformer\CollectionToArrayTransformer;
use Symfony\Bridge\Doctrine\Form\EventListener\MergeDoctrineCollectionListener;
use Symfony\Component\Form\AbstractType;
use Symfony\Component\Form\ChoiceList\ChoiceList;
use Symfony\Component\Form\ChoiceList\Factory\CachingFactoryDecorator;
use Symfony\Component\Form\Exception\RuntimeException;
use Symfony\Component\Form\FormBuilderInterface;
Expand All @@ -40,9 +41,9 @@ abstract class DoctrineType extends AbstractType implements ResetInterface
private $idReaders = [];

/**
* @var DoctrineChoiceLoader[]
* @var EntityLoaderInterface[]
*/
private $choiceLoaders = [];
private $entityLoaders = [];

/**
* Creates the label for a choice.
Expand Down Expand Up @@ -115,43 +116,26 @@ public function configureOptions(OptionsResolver $resolver)
$choiceLoader = function (Options $options) {
// Unless the choices are given explicitly, load them on demand
if (null === $options['choices']) {
$hash = null;
$qbParts = null;
// If there is no QueryBuilder we can safely cache
$vary = [$options['em'], $options['class']];

// If there is no QueryBuilder we can safely cache DoctrineChoiceLoader,
// also if concrete Type can return important QueryBuilder parts to generate
// hash key we go for it as well
if (!$options['query_builder'] || null !== $qbParts = $this->getQueryBuilderPartsForCachingHash($options['query_builder'])) {
$hash = CachingFactoryDecorator::generateHash([
$options['em'],
$options['class'],
$qbParts,
]);

if (isset($this->choiceLoaders[$hash])) {
return $this->choiceLoaders[$hash];
}
// hash key we go for it as well, otherwise fallback on the instance
if ($options['query_builder']) {
$vary[] = $this->getQueryBuilderPartsForCachingHash($options['query_builder']) ?? $options['query_builder'];
}

if (null !== $options['query_builder']) {
$entityLoader = $this->getLoader($options['em'], $options['query_builder'], $options['class']);
} else {
$queryBuilder = $options['em']->getRepository($options['class'])->createQueryBuilder('e');
$entityLoader = $this->getLoader($options['em'], $queryBuilder, $options['class']);
}

$doctrineChoiceLoader = new DoctrineChoiceLoader(
return ChoiceList::loader($this, new DoctrineChoiceLoader(
$options['em'],
$options['class'],
$options['id_reader'],
$entityLoader
);

if (null !== $hash) {
$this->choiceLoaders[$hash] = $doctrineChoiceLoader;
}

return $doctrineChoiceLoader;
$this->getCachedEntityLoader(
$options['em'],
$options['query_builder'] ?? $options['em']->getRepository($options['class'])->createQueryBuilder('e'),
$options['class'],
$vary
)
), $vary);
}

return null;
Expand All @@ -162,7 +146,7 @@ public function configureOptions(OptionsResolver $resolver)
// field name. We can only use numeric IDs as names, as we cannot
// guarantee that a non-numeric ID contains a valid form name
if ($options['id_reader'] instanceof IdReader && $options['id_reader']->isIntId()) {
return [__CLASS__, 'createChoiceName'];
return ChoiceList::fieldName($this, [__CLASS__, 'createChoiceName']);
}

// Otherwise, an incrementing integer is used as name automatically
Expand All @@ -176,7 +160,7 @@ public function configureOptions(OptionsResolver $resolver)
$choiceValue = function (Options $options) {
// If the entity has a single-column ID, use that ID as value
if ($options['id_reader'] instanceof IdReader && $options['id_reader']->isSingleId()) {
return [$options['id_reader'], 'getIdValue'];
return ChoiceList::value($this, [$options['id_reader'], 'getIdValue'], $options['id_reader']);
}

// Otherwise, an incrementing integer is used as value automatically
Expand Down Expand Up @@ -214,35 +198,21 @@ public function configureOptions(OptionsResolver $resolver)
// Set the "id_reader" option via the normalizer. This option is not
// supposed to be set by the user.
$idReaderNormalizer = function (Options $options) {
$hash = CachingFactoryDecorator::generateHash([
$options['em'],
$options['class'],
]);

// The ID reader is a utility that is needed to read the object IDs
// when generating the field values. The callback generating the
// field values has no access to the object manager or the class
// of the field, so we store that information in the reader.
// The reader is cached so that two choice lists for the same class
// (and hence with the same reader) can successfully be cached.
if (!isset($this->idReaders[$hash])) {
$classMetadata = $options['em']->getClassMetadata($options['class']);
$this->idReaders[$hash] = new IdReader($options['em'], $classMetadata);
}

if ($this->idReaders[$hash]->isSingleId()) {
return $this->idReaders[$hash];
}

return null;
return $this->getCachedIdReader($options['em'], $options['class']);
};

$resolver->setDefaults([
'em' => null,
'query_builder' => null,
'choices' => null,
'choice_loader' => $choiceLoader,
'choice_label' => [__CLASS__, 'createChoiceLabel'],
'choice_label' => ChoiceList::label($this, [__CLASS__, 'createChoiceLabel']),
'choice_name' => $choiceName,
'choice_value' => $choiceValue,
'id_reader' => null, // internal
Expand Down Expand Up @@ -274,6 +244,27 @@ public function getParent()

public function reset()
{
$this->choiceLoaders = [];
$this->entityLoaders = [];
}

private function getCachedIdReader(ObjectManager $manager, string $class): ?IdReader
{
$hash = CachingFactoryDecorator::generateHash([$manager, $class]);

if (isset($this->idReaders[$hash])) {
return $this->idReaders[$hash];
}

$idReader = new IdReader($manager, $manager->getClassMetadata($class));

// don't cache the instance for composite ids that cannot be optimized
return $this->idReaders[$hash] = $idReader->isSingleId() ? $idReader : null;
}

private function getCachedEntityLoader(ObjectManager $manager, $queryBuilder, string $class, array $vary): EntityLoaderInterface
{
$hash = CachingFactoryDecorator::generateHash($vary);

return $this->entityLoaders[$hash] ?? ($this->entityLoaders[$hash] = $this->getLoader($manager, $queryBuilder, $class));
}
}
24 changes: 12 additions & 12 deletions src/Symfony/Bridge/Doctrine/Tests/Form/Type/EntityTypeTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -1205,13 +1205,13 @@ public function testLoaderCaching()
'property3' => 2,
]);

$choiceLoader1 = $form->get('property1')->getConfig()->getOption('choice_loader');
$choiceLoader2 = $form->get('property2')->getConfig()->getOption('choice_loader');
$choiceLoader3 = $form->get('property3')->getConfig()->getOption('choice_loader');
$choiceList1 = $form->get('property1')->getConfig()->getAttribute('choice_list');
$choiceList2 = $form->get('property2')->getConfig()->getAttribute('choice_list');
$choiceList3 = $form->get('property3')->getConfig()->getAttribute('choice_list');

$this->assertInstanceOf('Symfony\Component\Form\ChoiceList\Loader\ChoiceLoaderInterface', $choiceLoader1);
$this->assertSame($choiceLoader1, $choiceLoader2);
$this->assertSame($choiceLoader1, $choiceLoader3);
$this->assertInstanceOf('Symfony\Component\Form\ChoiceList\LazyChoiceList', $choiceList1);
$this->assertSame($choiceList1, $choiceList2);
$this->assertSame($choiceList1, $choiceList3);
}

public function testLoaderCachingWithParameters()
Expand Down Expand Up @@ -1265,13 +1265,13 @@ public function testLoaderCachingWithParameters()
'property3' => 2,
]);

$choiceLoader1 = $form->get('property1')->getConfig()->getOption('choice_loader');
$choiceLoader2 = $form->get('property2')->getConfig()->getOption('choice_loader');
$choiceLoader3 = $form->get('property3')->getConfig()->getOption('choice_loader');
$choiceList1 = $form->get('property1')->getConfig()->getAttribute('choice_list');
$choiceList2 = $form->get('property2')->getConfig()->getAttribute('choice_list');
$choiceList3 = $form->get('property3')->getConfig()->getAttribute('choice_list');

$this->assertInstanceOf('Symfony\Component\Form\ChoiceList\Loader\ChoiceLoaderInterface', $choiceLoader1);
$this->assertSame($choiceLoader1, $choiceLoader2);
$this->assertSame($choiceLoader1, $choiceLoader3);
$this->assertInstanceOf('Symfony\Component\Form\ChoiceList\LazyChoiceList', $choiceList1);
$this->assertSame($choiceList1, $choiceList2);
$this->assertSame($choiceList1, $choiceList3);
}

protected function createRegistryMock($name, $em)
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Component/Form/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ CHANGELOG
5.1.0
-----

* Added a `ChoiceList` facade to leverage explicit choice list caching based on options
* Added an `AbstractChoiceLoader` to simplify implementations and handle global optimizations
* The `view_timezone` option defaults to the `model_timezone` if no `reference_date` is configured.
* Added default `inputmode` attribute to Search, Email and Tel form types.
Expand Down
135 changes: 135 additions & 0 deletions src/Symfony/Component/Form/ChoiceList/ChoiceList.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Form\ChoiceList;

use Symfony\Component\Form\ChoiceList\Factory\Cache\ChoiceAttr;
use Symfony\Component\Form\ChoiceList\Factory\Cache\ChoiceFieldName;
use Symfony\Component\Form\ChoiceList\Factory\Cache\ChoiceLabel;
use Symfony\Component\Form\ChoiceList\Factory\Cache\ChoiceLoader;
use Symfony\Component\Form\ChoiceList\Factory\Cache\ChoiceValue;
use Symfony\Component\Form\ChoiceList\Factory\Cache\GroupBy;
use Symfony\Component\Form\ChoiceList\Factory\Cache\PreferredChoice;
use Symfony\Component\Form\ChoiceList\Loader\CallbackChoiceLoader;
use Symfony\Component\Form\ChoiceList\Loader\ChoiceLoaderInterface;
use Symfony\Component\Form\FormTypeExtensionInterface;
use Symfony\Component\Form\FormTypeInterface;

/**
* A set of convenient static methods to create cacheable choice list options.
*
* @author Jules Pietri <jules@heahprod.com>
*/
final class ChoiceList
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I see the point of this class. I'd suggest removing it and inlining the instantiations instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the user land part of this feature, everything else is about optimising the internals, many advantages to me:

  • easier to document/learn, only one class instead of many (I also add the ChoiceFilter in [Form] Added a "choice_filter" option to ChoiceType #35733 and we may add more in the future like a ChoiceLabelAttr I'got locally)

  • better discovery since every factory method has its own specific phpdoc instead of the generic constructor in the abstract class

  • better auto complete, there are a lot of classes using the Choice word already:
    Screenshot 2020-02-17 at 7 08 15 PM

    VS

    Screenshot 2020-02-17 at 7 06 07 PM

I agree we could inline in our own usage in this PR to save a few static calls, but I wouldn't either since the types are some kind of documentation and given the Blackfire profiles I got with the current implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can see this class as a ChoiceList builder or configurator

{
/**
* Creates a cacheable loader from any callable providing iterable choices.
*
* @param FormTypeInterface|FormTypeExtensionInterface $formType A form type or type extension configuring a cacheable choice list
* @param callable $choices A callable that must return iterable choices or grouped choices
* @param mixed|null $vary Dynamic data used to compute a unique hash when caching the loader
*/
public static function lazy($formType, callable $choices, $vary = null): ChoiceLoader
{
return self::loader($formType, new CallbackChoiceLoader($choices), $vary);
}

/**
* Decorates a loader to make it cacheable.
*
* @param FormTypeInterface|FormTypeExtensionInterface $formType A form type or type extension configuring a cacheable choice list
* @param ChoiceLoaderInterface $loader A loader responsible for creating loading choices or grouped choices
* @param mixed|null $vary Dynamic data used to compute a unique hash when caching the loader
*/
public static function loader($formType, ChoiceLoaderInterface $loader, $vary = null): ChoiceLoader
{
return new ChoiceLoader($formType, $loader, $vary);
}

/**
* Decorates a "choice_value" callback to make it cacheable.
*
* @param FormTypeInterface|FormTypeExtensionInterface $formType A form type or type extension configuring a cacheable choice list
* @param callable $value Any pseudo callable to create a unique string value from a choice
* @param mixed|null $vary Dynamic data used to compute a unique hash when caching the callback
*/
public static function value($formType, $value, $vary = null): ChoiceValue
{
return new ChoiceValue($formType, $value, $vary);
}

/**
* Decorates a "choice_label" option to make it cacheable.
*
* @param FormTypeInterface|FormTypeExtensionInterface $formType A form type or type extension configuring a cacheable choice list
* @param callable|false $label Any pseudo callable to create a label from a choice or false to discard it
* @param mixed|null $vary Dynamic data used to compute a unique hash when caching the option
*/
public static function label($formType, $label, $vary = null): ChoiceLabel
{
return new ChoiceLabel($formType, $label, $vary);
}

/**
* Decorates a "choice_name" callback to make it cacheable.
*
* @param FormTypeInterface|FormTypeExtensionInterface $formType A form type or type extension configuring a cacheable choice list
* @param callable $fieldName Any pseudo callable to create a field name from a choice
* @param mixed|null $vary Dynamic data used to compute a unique hash when caching the callback
*/
public static function fieldName($formType, $fieldName, $vary = null): ChoiceFieldName
{
return new ChoiceFieldName($formType, $fieldName, $vary);
}

/**
* Decorates a "choice_attr" option to make it cacheable.
*
* @param FormTypeInterface|FormTypeExtensionInterface $formType A form type or type extension configuring a cacheable choice list
* @param callable|array $attr Any pseudo callable or array to create html attributes from a choice
* @param mixed|null $vary Dynamic data used to compute a unique hash when caching the option
*/
public static function attr($formType, $attr, $vary = null): ChoiceAttr
{
return new ChoiceAttr($formType, $attr, $vary);
}

/**
* Decorates a "group_by" callback to make it cacheable.
*
* @param FormTypeInterface|FormTypeExtensionInterface $formType A form type or type extension configuring a cacheable choice list
* @param callable $groupBy Any pseudo callable to return a group name from a choice
* @param mixed|null $vary Dynamic data used to compute a unique hash when caching the callback
*/
public static function groupBy($formType, $groupBy, $vary = null): GroupBy
{
return new GroupBy($formType, $groupBy, $vary);
}

/**
* Decorates a "preferred_choices" option to make it cacheable.
*
* @param FormTypeInterface|FormTypeExtensionInterface $formType A form type or type extension configuring a cacheable choice list
* @param callable|array $preferred Any pseudo callable or array to return a group name from a choice
* @param mixed|null $vary Dynamic data used to compute a unique hash when caching the option
*/
public static function preferred($formType, $preferred, $vary = null): PreferredChoice
{
return new PreferredChoice($formType, $preferred, $vary);
}

/**
* Should not be instantiated.
*/
private function __construct()
{
}
}
Loading
0