[Autocomplete]: implement group_by option on Entity Autocompleter results #481

jvancoillie · 2022-09-24T22:07:42Z

Q	A
Bug fix?	?
New feature?	yes
Tickets	Fix #477
License	MIT

Add possibility to group results (with group_by option) on ajax autocomplete result

weaverryan

Hi!

Really sorry for the slow review - this someone slipped past me! I've left some comments, but this is excellent work and I would love to incorporate it when you have some time to make these changes.

Thanks!

src/Autocomplete/src/AutocompleteResultsExecutor.php

src/Autocomplete/src/EntityAutocompleterInterface.php

src/Autocomplete/assets/src/controller.ts

jvancoillie · 2022-11-05T21:39:22Z

Hi @weaverryan 👋

Thank you for this feedback, I will make the changes following your comments.

jvancoillie · 2022-11-06T03:06:11Z

sorry for the small setback, I had some difficulties on the rebase of the branch 2.x :(

I had to do a force push because of my merge error.

jvancoillie · 2022-11-06T03:41:28Z

Here are the changes after rebase and the pagination supports.
There have been some changes, a review would be welcome. thanks

jvancoillie · 2023-02-07T08:56:32Z

Hi @weaverryan,

what should we do with this pr ? I have however tried to be reactive because we are waiting for this feature.

norkunas · 2023-02-07T10:44:28Z

Maybe if group_by is not configured, it should return the results in the old format? Because for me it would now be a BC break

weaverryan · 2023-02-07T16:17:58Z

Maybe if group_by is not configured, it should return the results in the old format? Because for me it would now be a BC break

I think that's an excellent idea. It would make the PR a lot safer to merge 👍

weaverryan

Sorry again for being late on this. Let us know if it is possible to use the "old format" if group_by is not set.

Cheers!

weaverryan · 2023-02-07T16:19:09Z

src/Autocomplete/src/EntityAutocompleterInterface.php

+    /**
+     * Return group_by option.
+     */
+    public function getGroupBy(): mixed;


Instead of physically adding this method, which would break BC, add some documentation to the top for it: https://github.com/SymfonyCasts/reset-password-bundle/blob/6601f15fce7a4934bc9570f76feaf1b93536b1a7/src/ResetPasswordHelperInterface.php#L19 - that will allow people to be aware of it, and then we will add the actual method whenever we release a new major version.

Also is the return value really mixed? Or is it ?string?

Yes i think, It can be string, callable or null

Ah, so it is: https://github.com/symfony/symfony/blob/7ed43d1a749d95ae3f3d8ffa9b72858cac3e39fd/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php#L370

That is QUITE the long list of things :). I think we could probably just use null|string|callable. Then, if someone passes a PropertyPath or GroupBy in a form, inside of WrappedEntityTypeAutocompleter, we could "normalize" those into a string|callable.

I'm struggling to see how to normalize this, how do you see the implementation of this normalization

jvancoillie · 2023-02-09T14:09:43Z

Hello, I made some changes to keep the old format if the group_by option is not set.

Would you have some time to give some feedback ?

src/Autocomplete/src/AutocompleteResults.php

weaverryan

This is VERY close now - let's push it across the finish line!

weaverryan · 2023-02-23T19:36:19Z

src/Autocomplete/CHANGELOG.md


+## 2.8.0
+
+-   Added support for using `Option Group`: the JSON format with option groupe takes two entries


Suggested change

- Added support for using `Option Group`: the JSON format with option groupe takes two entries

- Added support for using [OptionGroups](https://tom-select.js.org/examples/optgroups/).

And then I don't think we need to show the JSON format below. 99% of people won't care about this - it's just handled for them :)

weaverryan · 2023-02-23T19:43:45Z

src/Autocomplete/assets/src/controller.ts

            // '(query, callback) => { ... }' syntax because, otherwise,
            // the 'this.XXX' calls inside this method fail
-            load: function (query: string, callback: (results?: any) => void) {
+            load: function (query: string, callback: (options?: any, optgroups?: any) => void) {


After a little digging, it looks like this callback as a type:

Suggested change

load: function (query: string, callback: (options?: any, optgroups?: any) => void) {

load: function (query: string, callback: TomLoadCallback) {

The TomLoadCallback can be imported from 'tom-select/dist/types/types' like 3 other types already. Then, for the .catch() on line 154, it would become:

.catch(() => callback([], []));

to satisfy this type.

weaverryan · 2023-02-23T19:47:10Z

src/Autocomplete/doc/index.rst

+                    { "value": "1", "text": "Pizza" },
+                    { "value": "2", "text":"Banana"}
+                ]
+            }


This change is not valid anymore, right?

And we need docs for the new option :)

weaverryan · 2023-02-23T19:49:13Z

src/Autocomplete/src/AutocompleteResultsExecutor.php

-            $results[] = [
-                'value' => $autocompleter->getValue($entity),
-                'text' => $autocompleter->getLabel($entity),
-            ];


Minor, but I think we should "exit early" first if there is no group by. Basically:

if (null === $groupBy = $autocompleter->getGroupBy()) { // use the old logic $results = []; foreach ($paginator as $entity) { $results[] = [ 'value' => $autocompleter->getValue($entity), 'text' => $autocompleter->getLabel($entity), ]; } return $results; }

Then the code that handles $groupBy can continue without needing to be in an else statement (no else will be needed at all).

weaverryan · 2023-02-23T19:50:00Z

src/Autocomplete/src/EntityAutocompleterInterface.php

+    /**
+     * Return group_by option.
+     */
+    public function getGroupBy(): mixed;


Ah, so it is: https://github.com/symfony/symfony/blob/7ed43d1a749d95ae3f3d8ffa9b72858cac3e39fd/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php#L370

That is QUITE the long list of things :). I think we could probably just use null|string|callable. Then, if someone passes a PropertyPath or GroupBy in a form, inside of WrappedEntityTypeAutocompleter, we could "normalize" those into a string|callable.

weaverryan · 2023-02-23T20:16:02Z

src/Autocomplete/src/AutocompleteResultsExecutor.php

+            }
+        }
+
+        if (\is_callable($groupBy)) {


If it's not a callable at this point, that would be an error, right? I'm imagining that we check for null and string above (and for string, you're converting into a callable). So we could say:

if (!\is_callable($groupBy)) { // then throw an exception }

weaverryan · 2023-02-23T20:23:43Z

src/Autocomplete/src/AutocompleteResults.php

-        public array $results,
+        public array $options,
+        public array $optgroups,
        public bool $hasNextPage,


We can't break BC here, even though it's unlikely to cause problems. So:

A) Rename $options back to $results
B) Move $optgroups to the final argument and make it optional

Then, let's move the getResults() logic into the controller. That'll keep this class dead-simple - just a carrier of data.

weaverryan · 2023-02-23T20:24:28Z

src/Autocomplete/src/AutocompleteResultsExecutor.php

    public function __construct(
        private DoctrineRegistryWrapper $managerRegistry,
-        private ?Security $security = null
+        private PropertyAccessorInterface $propertyAccessor,


To protect BC and trigger a deprecation, we can make the top of the class look something like this:

final class AutocompleteResultsExecutor { private PropertyAccessorInterface $propertyAccessor; private ?Security $security; public function __construct( private DoctrineRegistryWrapper $managerRegistry, $propertyAccessor, /* Security $security = null */ ) { if ($propertyAccessor instanceof Security) { trigger_deprecation('symfony/ux-autocomplete', '2.8.0', 'Passing a "%s" instance as the second argument of "%s()" is deprecated, pass a "%s" instance instead.', Security::class, __METHOD__, PropertyAccessorInterface::class); $this->security = $propertyAccessor; $this->propertyAccessor = new PropertyAccessor(); } else { $this->propertyAccessor = $propertyAccessor; $this->security = func_num_args() >= 3 ? func_get_arg(2) : null; } }

…ults

weaverryan · 2023-04-14T13:59:37Z

Thank you @jvancoillie for this wonderful feature and your excellent work. Apologies for the slow merge on my end - but I'm super excited to happen (I just tried it locally and it was super fun)

weaverryan reviewed Nov 5, 2022

View reviewed changes

jvancoillie force-pushed the autocomplete_group_by branch from 57f8236 to c7996df Compare November 6, 2022 03:00

jvancoillie force-pushed the autocomplete_group_by branch from 7418dd6 to c5fe12a Compare November 6, 2022 03:37

weaverryan reviewed Feb 7, 2023

View reviewed changes

jvancoillie force-pushed the autocomplete_group_by branch from c5fe12a to 28c5cea Compare February 9, 2023 10:30

jvancoillie force-pushed the autocomplete_group_by branch from 9fb1380 to fcdafdd Compare February 9, 2023 14:11

norkunas reviewed Feb 9, 2023

View reviewed changes

src/Autocomplete/src/AutocompleteResults.php Outdated Show resolved Hide resolved

weaverryan reviewed Feb 23, 2023

View reviewed changes

[Autocomplete]: implement group_by option on Entity Autocompleter res…

a5b8c55

…ults

weaverryan force-pushed the autocomplete_group_by branch from 99069c0 to a5b8c55 Compare April 14, 2023 13:59

weaverryan merged commit c141baa into symfony:2.x Apr 14, 2023

1ed mentioned this pull request Apr 15, 2023

[Autocomplete] potential BC break in the upcoming 2.x release #793

Closed


		## 2.8.0

		- Added support for using `Option Group`: the JSON format with option groupe takes two entries

	- Added support for using `Option Group`: the JSON format with option groupe takes two entries
	- Added support for using [OptionGroups](https://tom-select.js.org/examples/optgroups/).

	load: function (query: string, callback: (options?: any, optgroups?: any) => void) {
	load: function (query: string, callback: TomLoadCallback) {

Uh oh!

[Autocomplete]: implement group_by option on Entity Autocompleter results #481

[Autocomplete]: implement group_by option on Entity Autocompleter results #481

Uh oh!

Conversation

jvancoillie commented Sep 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weaverryan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jvancoillie commented Nov 5, 2022

Uh oh!

jvancoillie commented Nov 6, 2022

Uh oh!

jvancoillie commented Nov 6, 2022

Uh oh!

jvancoillie commented Feb 7, 2023

Uh oh!

norkunas commented Feb 7, 2023

Uh oh!

weaverryan commented Feb 7, 2023

Uh oh!

weaverryan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvancoillie commented Feb 9, 2023

Uh oh!

Uh oh!

weaverryan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weaverryan commented Apr 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jvancoillie commented Sep 24, 2022 •

edited

Loading