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.

Sign up for GitHub

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

Jump to bottom

[Doctrine] SearchFilter - Use abitrary index instead of value #5079

Merged
merged 1 commit into from Oct 24, 2022
Merged

[Doctrine] SearchFilter - Use abitrary index instead of value #5079

merged 1 commit into from Oct 24, 2022

Conversation

Korbeil
Copy link
Contributor

@Korbeil Korbeil commented Oct 21, 2022 •
edited

Q A
Branch? 3.0
Tickets N/A
License MIT
Doc PR N/A

When using a search filter with multiple values that have the same lowercase identity, for example:

array:2 [▼
  0 => "rayon bleu"
  1 => "Rayon Bleu"
]

SearchFilter will lowercase the value and use it as index key, which makes it send only one parameter to Doctrine when it needs two of them.

I choosed to leave the arbitrary array index so we don't have this kind of issues anymore.

@Korbeil Korbeil force-pushed the fix/parameter-not-set branch 3 times, most recently from 0c4a2ed to 729aca1 Compare October 21, 2022 10:04
@alanpoulain
Copy link
Member

Could you please add unit tests?

@Korbeil Korbeil force-pushed the fix/parameter-not-set branch 2 times, most recently from fa45aac to 9ca1f93 Compare October 21, 2022 12:40
@Korbeil
Copy link
Contributor Author

Korbeil commented Oct 21, 2022

Could you please add unit tests?

Is that enough ? Not sure what I could add more 🤔

@alanpoulain
Copy link
Member

Is that enough ? Not sure what I could add more thinking

Yes, that's enough 🙂 You need to change the SearchFilterTest for MongoDB too (sorry for that, please tell me if you want me to do it).

@bastnic
Copy link
Contributor

bastnic commented Oct 21, 2022

nice catch

@soyuka
Copy link
Member

soyuka commented Oct 21, 2022 •
edited

how was this in 2.6 ? I think that we could fix this on 3.0 instead, 2.7 should have only bc-layer related fixes. Unless this was wrongly introduced in 2.7 we may fix it here.

@Korbeil
Copy link
Contributor Author

Korbeil commented Oct 21, 2022

how was this in 2.6 ? I think that we could fix this on 3.0 instead, 2.7 should have only bc-layer related fixes. Unless this was wrongly introduced in 2.7 we may fix it here.

I do think this was here since a long time, I'll change the branch to 3.0 then.

@Korbeil Korbeil changed the base branch from 2.7 to 3.0 October 21, 2022 14:13
@soyuka soyuka merged commit 7044c5a into api-platform:3.0 Oct 24, 2022
@soyuka
Copy link
Member

soyuka commented Oct 24, 2022

Thanks @Korbeil !

@Korbeil Korbeil deleted the fix/parameter-not-set branch October 24, 2022 09:14
soyuka added a commit that referenced this pull request Nov 4, 2022
@soyuka @ArnoudThibaut
@davy-beauzil @Korbeil @norival @alanpoulain @liviucmg @WilliamPeralta
Merge 3.0 into main (#5121)
6abd0fe 
* fix: update yaml extractor test file coding standard (#5068)

* fix(graphql): add clearer error message when TwigBundle is disabled but graphQL clients are enabled (#5064)

* fix(metadata): add class key in payload argument resolver (#5067)

* fix: add class key in payload argument resolver

* add null if everything else goes wrong

* fix: upgrade command remove ApiSubresource attribute  (#5049)

Fixes #5038

* fix(doctrine): use abitrary index instead of value (#5079)

* fix: uri template should respect rfc 6570 (#5080)

* fix: remove @internal annotation for Operations (#5089)

See #5084

* fix(metadata): define a name on a single operation (#5090)

fixes #5082

* fix(metadata): deprecate when user decorates in legacy mode (#5091)

fixes #5078

* fix(graphql): use right nested operation (#5102)

* Revert "fix(graphql): use right nested operation (#5102)" (#5111)

This reverts commit 44337dd.

* fix(graphql): always allow to query nested resources (#5112)

* fix(graphql): always allow to query nested resources

* review

Co-authored-by: Alan Poulain <contact@alanpoulain.eu>

* chore: php-cs-fixer update

* fix: only alias if exists for opcache preload

Fixes api-platform/api-platform#2284 (#5110)

Co-authored-by: Liviu Mirea <liviu.mirea@wecodepixels.com>

* chore: php-cs-fixer update (#5118)

* chore: php-cs-fixer update

* chore: php-cs-fixer update

* fix(metadata): upgrade script keep operation name (#5109)

origin: #5105

Co-authored-by: WilliamPeralta <william.peralta18@gmail.com>

* chore: v2.7.3 changelog

* chore: v3.0.3 changelog

Co-authored-by: helyakin <CourcierMarvin@gmail.com>
Co-authored-by: ArnoudThibaut <thibaut.arnoud@gmail.com>
Co-authored-by: davy-beauzil <38990335+davy-beauzil@users.noreply.github.com>
Co-authored-by: Baptiste Leduc <baptiste.leduc@gmail.com>
Co-authored-by: Xavier Laviron <norival@users.noreply.github.com>
Co-authored-by: Alan Poulain <contact@alanpoulain.eu>
Co-authored-by: Liviu Cristian Mirea-Ghiban <contact@liviucmg.com>
Co-authored-by: Liviu Mirea <liviu.mirea@wecodepixels.com>
Co-authored-by: WilliamPeralta <william.peralta18@gmail.com>
alexndlm added a commit to eonx-com/easy-monorepo that referenced this pull request Nov 7, 2022
@alexndlm
-sync with api-platform/core#5079
35c5bc9
alexndlm added a commit to eonx-com/easy-monorepo that referenced this pull request Nov 18, 2022
@alexndlm
-sync with api-platform/core#5079
efe6be3
alexndlm added a commit to eonx-com/easy-monorepo that referenced this pull request Jan 13, 2023
@alexndlm
-sync with api-platform/core#5079
0d3cd6e
natepage pushed a commit to eonx-com/easy-monorepo that referenced this pull request May 21, 2023
@alexndlm
[EasyApiPlatform] Create AdvancedSearchFilter (#1059)
194c041 
* -create AdvancedSearchFilter

* -fix dependencies

* -fix dependencies

* -fix dependencies

* -fix dependencies

* -fix dependencies

* -fix dependencies

* -rollback

* -sync with api-platform/core#5079

* -fix ecs

* -fix typo

* -refactoring

* -fix ecs

* -refactoring

* -fix phpstan

* -refactoring

* -fix ecs

* -update composer.json

* -update composer.json

* -rename Stubs to Fixtures

* -rename Stubs to Fixtures

* -rename Stubs to Fixtures

* -refactoring

* -refactoring

* -fix phpstan

* -rollback changes

* -fix deps
natepage pushed a commit to eonx-com/easy-api-platform that referenced this pull request May 21, 2023
@alexndlm
[EasyApiPlatform] Create AdvancedSearchFilter (#1059)
3397a55 
* -create AdvancedSearchFilter

* -fix dependencies

* -fix dependencies

* -fix dependencies

* -fix dependencies

* -fix dependencies

* -fix dependencies

* -rollback

* -sync with api-platform/core#5079

* -fix ecs

* -fix typo

* -refactoring

* -fix ecs

* -refactoring

* -fix phpstan

* -refactoring

* -fix ecs

* -update composer.json

* -update composer.json

* -rename Stubs to Fixtures

* -rename Stubs to Fixtures

* -rename Stubs to Fixtures

* -refactoring

* -refactoring

* -fix phpstan

* -rollback changes

* -fix deps
natepage pushed a commit to eonx-com/easy-core that referenced this pull request May 21, 2023
@alexndlm
[EasyApiPlatform] Create AdvancedSearchFilter (#1059)
5ac4503 
* -create AdvancedSearchFilter

* -fix dependencies

* -fix dependencies

* -fix dependencies

* -fix dependencies

* -fix dependencies

* -fix dependencies

* -rollback

* -sync with api-platform/core#5079

* -fix ecs

* -fix typo

* -refactoring

* -fix ecs

* -refactoring

* -fix phpstan

* -refactoring

* -fix ecs

* -update composer.json

* -update composer.json

* -rename Stubs to Fixtures

* -rename Stubs to Fixtures

* -rename Stubs to Fixtures

* -refactoring

* -refactoring

* -fix phpstan

* -rollback changes

* -fix deps
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@welcoMattic welcoMattic welcoMattic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Korbeil @alanpoulain @bastnic @soyuka @welcoMattic