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.

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

fix(entity): Fix mapping of old/sub-types to actually supported datab… #48837

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

nickvergessen
Copy link
Member

…ase types

Checklist

@nickvergessen nickvergessen added the 3. to review Waiting for reviews label Oct 22, 2024
@nickvergessen nickvergessen added this to the Nextcloud 31 milestone Oct 22, 2024
@nickvergessen nickvergessen self-assigned this Oct 22, 2024
@@ -263,6 +272,17 @@ public function getUpdatedFields(): array {
* @since 7.0.0
*/
protected function addType(string $fieldName, string $type): void {
if (in_array($type, ['bool', 'double', 'int', 'array', 'object'], true)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we document those to satisfy psalm? Or yolo?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the options are this, right?

  1. Document and make it somewhat official by doing so
  2. yolo and apps have to baseline or use the types from the constants

I would prefer to fix the apps (so 2), but not sure if this is too much effort and I do not want to make all devs angry...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a @since entry to the method to explain it, together with a psalm-suppress.
We could backport the setter() change, to make sure using Types::TEXT and the others works also on stable branches. But then the apps would have to depend on a specific patch release or still need a version switch.

So for now I would say they will have to use the psalm baseline until they dropped support for everything < 31 and then move to Types?

Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get this in quickly please 🙏 Thanks!

CI is red in all Groupware apps and we are kind of blocked until a decision is made on this.

EDIT: Or we simply migrate to 'integer' which shouldn't be breaking anything, right?

@nickvergessen nickvergessen force-pushed the followup/47329/add-all-types-to-handling branch from 6c86492 to 0864428 Compare October 22, 2024 10:37
@nickvergessen
Copy link
Member Author

CI is red in all Groupware apps and we are kind of blocked until a decision is made on this.

Unless you migrate to the new consts psalm will stay red, but yeah same issue in talk

@nickvergessen nickvergessen force-pushed the followup/47329/add-all-types-to-handling branch from 0864428 to 8810d78 Compare October 23, 2024 07:04
@nickvergessen nickvergessen force-pushed the followup/47329/add-all-types-to-handling branch from 8810d78 to f973616 Compare October 23, 2024 07:20
@nickvergessen nickvergessen force-pushed the followup/47329/add-all-types-to-handling branch from f973616 to 54c3aa3 Compare October 23, 2024 07:22
@nickvergessen nickvergessen merged commit 8f59997 into master Oct 23, 2024
175 of 177 checks passed
@nickvergessen nickvergessen deleted the followup/47329/add-all-types-to-handling branch October 23, 2024 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants