Skip to content

fix(Pagination): Fix UUID in range filter - #4595

Closed
KDederichs wants to merge 19 commits into
api-platform:mainfrom
KDederichs:pagination_fix
Closed

fix(Pagination): Fix UUID in range filter#4595
KDederichs wants to merge 19 commits into
api-platform:mainfrom
KDederichs:pagination_fix

Conversation

@KDederichs

Copy link
Copy Markdown
Contributor
QA
Branch?main (cause I used the new project structure)
Ticketsapi-platform/core#8033
LicenseMIT
Doc PRnone

Turns out the things I was running into were a bug caused by the range filter expecting numbers.
Which doesn't work when you use UUIDs as PK :)

Comment threadsrc/Doctrine/Common/Filter/RangeFilterTrait.php Outdated
Comment threadsrc/Doctrine/Common/Filter/RangeFilterTrait.php
Comment threadsrc/Doctrine/Common/Filter/RangeFilterTrait.php Outdated
@KDederichs

Copy link
Copy Markdown
ContributorAuthor

Ok the behat 7.1 fails cause the test can't init the UUID v6 objects.
I'm not really familiar with behat, is there a way to skip a test if a class is missing?

@KDederichs

Copy link
Copy Markdown
ContributorAuthor

Or better why still test for 7.1?
It's long past EOL

@Chris53897

Copy link
Copy Markdown
Contributor

Or better why still test for 7.1? It's long past EOL

Symfony backwards compatibility promise.
Symfony 4.4 php 7.1

@KDederichs

Copy link
Copy Markdown
ContributorAuthor

Ok skipping the test it is then (took me a while to figure out how to do that in behat but I gave it a custom tag that can be removed when 7.1 is dropped

@KDederichsKDederichs changed the title Fix UUID in rangefix(Pagination): Fix UUID in range filterDec 22, 2021
@KDederichs

Copy link
Copy Markdown
ContributorAuthor

All green except deprecations 👍

@soyuka

Copy link
Copy Markdown
Member

This should be moved to its own filter.

@KDederichs

Copy link
Copy Markdown
ContributorAuthor

@soyuka should it though?
You can also have a range on a string imo. But if you want I can close this and make a new PR that introduces something like UuidRange or something and then also adds that to the docs.

@KDederichs

Copy link
Copy Markdown
ContributorAuthor

Closing in favour of #4689
@soyuka if you'd take a look at that one I'd be grateful :)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@KDederichs@Chris53897@soyuka@alanpoulain