-
-
Notifications
You must be signed in to change notification settings - Fork 324
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
[Autocomplete] Allow passing extra options to the autocomplete fields #1322
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,39 @@ | ||||||||||||||
<?php | ||||||||||||||
|
||||||||||||||
declare(strict_types=1); | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not usually set in symfony components AFAIK (sorry for the c/c) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @weaverryan should I remove it? I've added it as I have it in my PHP class template in the IDE (as I consider it as a good practice) 🤔. |
||||||||||||||
|
||||||||||||||
/* | ||||||||||||||
* This file is part of the Symfony package. | ||||||||||||||
* | ||||||||||||||
* (c) Fabien Potencier <[email protected]> | ||||||||||||||
* | ||||||||||||||
* For the full copyright and license information, please view the LICENSE | ||||||||||||||
* file that was distributed with this source code. | ||||||||||||||
*/ | ||||||||||||||
|
||||||||||||||
namespace Symfony\UX\Autocomplete\Checksum; | ||||||||||||||
|
||||||||||||||
/** @internal */ | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
(same thing on OptionsAware..Interface ?) |
||||||||||||||
class ChecksumCalculator | ||||||||||||||
{ | ||||||||||||||
public function __construct(private readonly string $secret) | ||||||||||||||
{ | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
public function calculateForArray(array $data): string | ||||||||||||||
{ | ||||||||||||||
$this->sortKeysRecursively($data); | ||||||||||||||
|
||||||||||||||
return base64_encode(hash_hmac('sha256', json_encode($data), $this->secret, true)); | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe use some lighter hash algo?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would the base64 still be necessary ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The question here would be how we treat that data, for me it's checksum which is not a password but should be kinda of cryptographic, but it doesn't need to be perfect as a password and it doesn't need to be as safe as it is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, you're right. I thought (idk why :D) that we encrypt data here, but the data is not encrypted at all. So the answer here is simple, I used There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now I'm thinking about extracting checksum calculating inside the LiveComponent component, and using it in the autocomplete component. Autocomplete depends on the LiveComponent so it might make sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would make perfect sense to me. Feel free to do it in a second/later PR if you want to release this one first... in all maner this should be "internal" so there is no BC involved there :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was 100% sure So, for now, I leave the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'd prefer to remove the interface. I can't see a legit reason to need to write your own. If there is a problem with our class, then we want them to tell us so we can fix it :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @weaverryan I've removed it! |
||||||||||||||
} | ||||||||||||||
|
||||||||||||||
private function sortKeysRecursively(array &$data): void | ||||||||||||||
{ | ||||||||||||||
foreach ($data as &$value) { | ||||||||||||||
if (\is_array($value)) { | ||||||||||||||
$this->sortKeysRecursively($value); | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
ksort($data); | ||||||||||||||
} | ||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,20 +14,27 @@ | |
use Symfony\Component\HttpFoundation\JsonResponse; | ||
use Symfony\Component\HttpFoundation\Request; | ||
use Symfony\Component\HttpFoundation\Response; | ||
use Symfony\Component\HttpKernel\Exception\BadRequestHttpException; | ||
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; | ||
use Symfony\Component\Routing\Generator\UrlGeneratorInterface; | ||
use Symfony\UX\Autocomplete\AutocompleteResultsExecutor; | ||
use Symfony\UX\Autocomplete\AutocompleterRegistry; | ||
use Symfony\UX\Autocomplete\Checksum\ChecksumCalculator; | ||
use Symfony\UX\Autocomplete\Form\AutocompleteChoiceTypeExtension; | ||
use Symfony\UX\Autocomplete\OptionsAwareEntityAutocompleterInterface; | ||
|
||
/** | ||
* @author Ryan Weaver <[email protected]> | ||
*/ | ||
final class EntityAutocompleteController | ||
{ | ||
public const EXTRA_OPTIONS = 'extra_options'; | ||
|
||
public function __construct( | ||
private AutocompleterRegistry $autocompleteFieldRegistry, | ||
private AutocompleteResultsExecutor $autocompleteResultsExecutor, | ||
private UrlGeneratorInterface $urlGenerator, | ||
private ChecksumCalculator $checksumCalculator, | ||
) { | ||
} | ||
|
||
|
@@ -38,6 +45,11 @@ public function __invoke(string $alias, Request $request): Response | |
throw new NotFoundHttpException(sprintf('No autocompleter found for "%s". Available autocompleters are: (%s)', $alias, implode(', ', $this->autocompleteFieldRegistry->getAutocompleterNames()))); | ||
} | ||
|
||
if ($autocompleter instanceof OptionsAwareEntityAutocompleterInterface) { | ||
$extraOptions = $this->getExtraOptions($request); | ||
$autocompleter->setOptions([self::EXTRA_OPTIONS => $extraOptions]); | ||
} | ||
|
||
$page = $request->query->getInt('page', 1); | ||
$nextPage = null; | ||
|
||
|
@@ -54,4 +66,48 @@ public function __invoke(string $alias, Request $request): Response | |
'next_page' => $nextPage, | ||
]); | ||
} | ||
|
||
/** | ||
* @return array<string, scalar|array|null> | ||
*/ | ||
private function getExtraOptions(Request $request): array | ||
{ | ||
if (!$request->query->has(self::EXTRA_OPTIONS)) { | ||
return []; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we still detect if anyone removes the extra options ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It simplifies the code. The method always returns an array, so no additional checks are required method "above". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My scenario was: if someone manually removes the extra_options paramters in the query string, and we expect some to be defined, what would happen ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤷🏼♂️ I believe it's not a big deal to not check whether an option was set (and I guess it might be a little bit problematic to check that), but maybe I'm too lax on this topic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok ! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if we used Symfony's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've started reimplementing this feature, and... I noticed it computes the hash in the same way as we calculate the checksum :D.
I have troubles with testing |
||
} | ||
|
||
$extraOptions = $this->getDecodedExtraOptions($request->query->getString(self::EXTRA_OPTIONS)); | ||
|
||
if (!\array_key_exists(AutocompleteChoiceTypeExtension::CHECKSUM_KEY, $extraOptions)) { | ||
throw new BadRequestHttpException('The extra options are missing the checksum.'); | ||
} | ||
|
||
$this->validateChecksum($extraOptions[AutocompleteChoiceTypeExtension::CHECKSUM_KEY], $extraOptions); | ||
|
||
return $extraOptions; | ||
} | ||
|
||
/** | ||
* @return array<string, scalar> | ||
*/ | ||
private function getDecodedExtraOptions(string $extraOptions): array | ||
{ | ||
return json_decode(base64_decode($extraOptions), true, flags: \JSON_THROW_ON_ERROR); | ||
} | ||
|
||
/** | ||
* @param array<string, scalar> $extraOptions | ||
*/ | ||
private function validateChecksum(string $checksum, array $extraOptions): void | ||
{ | ||
$extraOptionsWithoutChecksum = array_filter( | ||
$extraOptions, | ||
fn (string $key) => AutocompleteChoiceTypeExtension::CHECKSUM_KEY !== $key, | ||
\ARRAY_FILTER_USE_KEY, | ||
); | ||
|
||
if ($checksum !== $this->checksumCalculator->calculateForArray($extraOptionsWithoutChecksum)) { | ||
throw new BadRequestHttpException('The extra options have been tampered with.'); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the Symfony package. | ||
* | ||
* (c) Fabien Potencier <[email protected]> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
namespace Symfony\UX\Autocomplete; | ||
|
||
/** | ||
* Interface for classes that will have an "autocomplete" endpoint exposed with a possibility to pass additional form options. | ||
*/ | ||
interface OptionsAwareEntityAutocompleterInterface extends EntityAutocompleterInterface | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather have separate Interfaces, what do you think ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do like creating "subtype" interfaces like this, but it's not a thing I'd fight for :D. I'm open for changing it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No strong opinion on this. i'm often more concerned about "what could happen later" than i probably should... i'm working on it but habits are hard to change :) That's why i ask / raise questions but would never "require" changes :)) |
||
{ | ||
public function setOptions(array $options): void; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,15 @@ | ||
<?php | ||
|
||
namespace Symfony\UX\Autocomplete\Tests\Fixtures\Autocompleter; | ||
/* | ||
* This file is part of the Symfony package. | ||
* | ||
* (c) Fabien Potencier <[email protected]> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
use Doctrine\ORM\EntityRepository; | ||
use Doctrine\ORM\QueryBuilder; | ||
use Symfony\Component\HttpFoundation\RequestStack; | ||
use Symfony\Bundle\SecurityBundle\Security; | ||
use Symfony\UX\Autocomplete\Doctrine\EntitySearchUtil; | ||
use Symfony\UX\Autocomplete\EntityAutocompleterInterface; | ||
use Symfony\UX\Autocomplete\Tests\Fixtures\Entity\Product; | ||
namespace Symfony\UX\Autocomplete\Tests\Fixtures\Autocompleter; | ||
|
||
class CustomGroupByProductAutocompleter extends CustomProductAutocompleter | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,20 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the Symfony package. | ||
* | ||
* (c) Fabien Potencier <[email protected]> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
namespace Symfony\UX\Autocomplete\Tests\Fixtures\Autocompleter; | ||
|
||
use Doctrine\ORM\EntityRepository; | ||
use Doctrine\ORM\QueryBuilder; | ||
use Symfony\Component\HttpFoundation\RequestStack; | ||
use Symfony\Bundle\SecurityBundle\Security; | ||
use Symfony\Component\HttpFoundation\RequestStack; | ||
use Symfony\UX\Autocomplete\Doctrine\EntitySearchUtil; | ||
use Symfony\UX\Autocomplete\EntityAutocompleterInterface; | ||
use Symfony\UX\Autocomplete\Tests\Fixtures\Entity\Product; | ||
|
@@ -14,9 +23,8 @@ class CustomProductAutocompleter implements EntityAutocompleterInterface | |
{ | ||
public function __construct( | ||
private RequestStack $requestStack, | ||
private EntitySearchUtil $entitySearchUtil | ||
) | ||
{ | ||
private EntitySearchUtil $entitySearchUtil, | ||
) { | ||
} | ||
|
||
public function getEntityClass(): string | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compared to symfony/form changelog, i'd write it maybe more like
extra_options
to XXX and YYY types (those options are .... to ...)