- Issue created by @Quentin Massez
- Issue was unassigned.
- Status changed to Needs review
3 months ago 12:36pm 9 September 2024 - Status changed to Needs work
3 months ago 1:05pm 9 September 2024 - π¨π¦Canada Liam Morland Ontario, CA π¨π¦
Thanks for the patch. Please make a merge request and indent with spaces. Can you add a test that would have caught this?
- First commit to issue fork.
- Merge request !517webform-3473045: Fix Autocomplete STARTS_WITH β (Merged) created by PabloNicolas
- π§π·Brazil PabloNicolas
Hello there!
I tested the patch and can confirm it is working as expected! The merge request uses spaces rather than TAB to indent, I might need more time to work on the tests, but I hope to commit something by tomorrow.
Thank you!
- π¨π¦Canada Liam Morland Ontario, CA π¨π¦
It doesn't look to me like this patch changes anything. It just divides up the conditional into two
if
statements. - π§π·Brazil PabloNicolas
At first glance, I had the same thought as you, but the difference when we split it into a nested
if
is that the logic does not fall on theelseif
block. This block just validates the query is not FALSE and uses the CONTAINS operator.
elseif (stripos($label, $q) !== FALSE)
Another suggestion would be validating that the operator is empty or equals CONTAINS. Both solutions achieve the same result. - π¨π¦Canada Liam Morland Ontario, CA π¨π¦
Yes, I see. It would probably be better to use a
switch
statement for the value of $operator and thenif
statements within that. - π§π·Brazil PabloNicolas
Cool, just using the switch statement was not enough, but putting a
break;
on STARTS_WITH case does the trick.About testing, I'm not very used to writing unit tests, following the existing ones I added a few lines and it seemed correct, but I could not test it using PHPUnit using ddev :/
If someone could shed light on how to do the tests properly I would give it a shot.
Thank you!
- π§π·Brazil PabloNicolas
Hello Liam.
It took me a little while to figure out how to run the tests on my local environments and IDE, but now we have tests for this specific operator.
Do you mind having a look?
PS: For those having the same difficulties I had with tests, here is an excellent article on how to integrate Unit tests with DDEV and PHP Storm: https://drupalize.me/blog/debug-any-drupals-phpunit-tests-phpstorm-ddev-...
- π¨π¦Canada Liam Morland Ontario, CA π¨π¦
Tests are not passing.
- πΊπΈUnited States jrockowitz Brooklyn, NY
Using a switch/case could make the code cleaner and prevent the issue.
switch ($operator) { case 'STARTS_WITH': if (str_starts_with($label, $q)) { $matches[$label] = [ 'value' => $label, 'label' => $label, ]; } break; case 'CONTAINS': default: if (str_contains($label, $q)) { $matches[$label] = [ 'value' => $label, 'label' => $label, ]; } break; } }
- π§π·Brazil PabloNicolas
The tests are passing now, also fixed some deprecated code and PHPCS warnings.
- π¨π¦Canada Liam Morland Ontario, CA π¨π¦
Thanks for the patch. Please move the changes regarding
::willReturnCallback()
to a separate issue and merge request. - π§π·Brazil PabloNicolas
pablonicolas β changed the visibility of the branch 3473045-fix-starts-with-autocompletion to hidden.
- π§π·Brazil PabloNicolas
pablonicolas β changed the visibility of the branch 3473045-autocomplete-field-with to hidden.
- π§π·Brazil PabloNicolas
Done @liam morland. The merge request now only contains commits linked to this specific issue. I will create a new issue soon to fix the PHPCS and PHPCBF warnings.
Thank you!
-
jrockowitz β
committed f2bd43de on 6.3.x authored by
pablonicolas β
Issue #3473045: Autocomplete field with "STARTS_WITH" enabled
-
jrockowitz β
committed f2bd43de on 6.3.x authored by
pablonicolas β
- Status changed to Fixed
25 days ago 12:39am 27 November 2024 -
jrockowitz β
committed f2bd43de on 6.x authored by
pablonicolas β
Issue #3473045: Autocomplete field with "STARTS_WITH" enabled
-
jrockowitz β
committed f2bd43de on 6.x authored by
pablonicolas β
Automatically closed - issue fixed for 2 weeks with no activity.