Skip to content

[Improvement]: Allow filter definition's page limit to be int as well #245

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

Merged
merged 1 commit into from
Apr 17, 2025

Conversation

dolmit-tanel-paaro
Copy link
Contributor

Resolves #195

@dolmit-tanel-paaro dolmit-tanel-paaro changed the title [Improvement]: Change filter definition's page limit from float to int [Improvement] Change filter definition's page limit from float to int Apr 11, 2025
@dolmit-tanel-paaro dolmit-tanel-paaro changed the title [Improvement] Change filter definition's page limit from float to int [Improvement]: Change filter definition's page limit from float to int Apr 11, 2025
@fashxp
Copy link
Member

fashxp commented Apr 11, 2025

nice, do we need a migration there for existing data?

@fashxp fashxp added this to the 2.0.0 milestone Apr 11, 2025
@dolmit-tanel-paaro
Copy link
Contributor Author

No, existing decimal values will get converted to integers, meaning nothing really happens to the values.
But everyone will have to update their FilterDefinition class definition to support the new return typehint.

@fashxp
Copy link
Member

fashxp commented Apr 14, 2025

But everyone will have to update their FilterDefinition class definition to support the new return typehint.

That's also what I meant with migration :-). At least we will need an upgrade note. But I'm not sure though, how a migration path could look like?

Automatically overwriting the class definition is dangerous, because there might custom adaption to that class definition.
Before the upgrade you won't be able to manually change the typehint, because then they won't match to the old abstract classes anymore and break the system.
After a composer update the system also will be broken for the same reason - the typehints do not match to the abstract classes anymore.

Any ideas?

@dolmit-tanel-paaro
Copy link
Contributor Author

dolmit-tanel-paaro commented Apr 14, 2025

Yeah, this situation is just like the one we faced when we migrated from Pimcore 6 to Pimcore X. Everything was broken because of return typehints because they were made mandatory.

Back then we solved it by upgrading to Pimcore X and then manually go over our class definitions and update return typehints manually.

The migration plan:

  • update the package
  • fix the issue(s) manually
  • run composer again

@fashxp
Copy link
Member

fashxp commented Apr 17, 2025

Hmm, we could do a two step process.
for now, we just change return-type of interface to float|int|null, which won't break any existing installations and also does not require any changes on existing data - but allows safely to convert the data over time.

And then with next major, we could switch to pure int.

The change in src/Resources/install/class_sources/class_FilterDefinition_export.json we could keep, as it just reflects for new installations.

@dolmit-tanel-paaro
Copy link
Contributor Author

Yeah, that's a good approach. I'll make the changes.

@dolmit-tanel-paaro dolmit-tanel-paaro force-pushed the filter-definition-page-limit branch from 2d4e4d5 to a6bb199 Compare April 17, 2025 08:13
Copy link

@dolmit-tanel-paaro
Copy link
Contributor Author

dolmit-tanel-paaro commented Apr 17, 2025

Updated abstract class, allowing float and int values.
Added todo for next major.
diff

@fashxp fashxp removed the BC-BREAK label Apr 17, 2025
@fashxp
Copy link
Member

fashxp commented Apr 17, 2025

thx very much

@fashxp fashxp merged commit 7ed8eb3 into pimcore:2.x Apr 17, 2025
13 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 17, 2025
@fashxp fashxp changed the title [Improvement]: Change filter definition's page limit from float to int [Improvement]: Allow filter definition's page limit to be int as well Apr 17, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement]: Change filter definition's page limit from float to int
2 participants