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

Replace DC2TYPE:array field types with JSON #16008

Merged
merged 4 commits into from Mar 21, 2024
Merged

Conversation

TheMilek
Copy link
Member

@TheMilek TheMilek commented Mar 15, 2024

Q A
Branch? 1.13
Bug fix? no
New feature? no
BC breaks? no
Related tickets fixes #9399
License MIT

@TheMilek TheMilek added the Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). label Mar 15, 2024
@TheMilek TheMilek requested review from a team as code owners March 15, 2024 12:10
@probot-autolabeler probot-autolabeler bot added the Maintenance CI configurations, READMEs, releases, etc. label Mar 15, 2024
Copy link

github-actions bot commented Mar 15, 2024

Bunnyshell Preview Environment deleted

Available commands:

  • /bns:deploy to redeploy the environment

@TheMilek TheMilek force-pushed the SYL-2874 branch 7 times, most recently from 438b422 to c0d4e19 Compare March 18, 2024 13:10
jakubtobiasz
jakubtobiasz previously approved these changes Mar 18, 2024
@@ -22,7 +22,7 @@
<field name="objectId" column="object_id" length="64" nullable="true" />
<field name="objectClass" column="object_class" type="string" />
<field name="version" column="version" type="integer" />
<field name="data" column="data" type="array" />
Copy link
Member

Choose a reason for hiding this comment

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

Why nullable 🤔?

Copy link
Member

Choose a reason for hiding this comment

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

I guess it was done to match original mapping

https://github.com/doctrine-extensions/DoctrineExtensions/blob/8d658b4d22977e3b72f02bfe4e68b2df0ba586aa/src/Loggable/Entity/MappedSuperclass/AbstractLogEntry.php#L83-L89

but I don't think it makes sense to do so at this stage.

Suggested change
<field name="data" column="data" type="array" />
<field name="data" column="data" type="json" />

Copy link
Member Author

Choose a reason for hiding this comment

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

I need to leave this change, as without it the removal of address'es is not working
Screenshot 2024-03-19 at 08 43 13

Copy link
Member Author

Choose a reason for hiding this comment

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

this scenario for ref

Copy link
Member

Choose a reason for hiding this comment

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

Strange, that it worked before.

What do you think mentioning this change in changelog, if it's necessary?

UPGRADE-1.13.md Show resolved Hide resolved
@@ -22,7 +22,7 @@
<field name="objectId" column="object_id" length="64" nullable="true" />
<field name="objectClass" column="object_class" type="string" />
<field name="version" column="version" type="integer" />
<field name="data" column="data" type="array" />
Copy link
Member

Choose a reason for hiding this comment

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

I guess it was done to match original mapping

https://github.com/doctrine-extensions/DoctrineExtensions/blob/8d658b4d22977e3b72f02bfe4e68b2df0ba586aa/src/Loggable/Entity/MappedSuperclass/AbstractLogEntry.php#L83-L89

but I don't think it makes sense to do so at this stage.

Suggested change
<field name="data" column="data" type="array" />
<field name="data" column="data" type="json" />

NoResponseMate
NoResponseMate previously approved these changes Mar 20, 2024
UPGRADE-1.13.md Outdated Show resolved Hide resolved
diimpp
diimpp previously approved these changes Mar 20, 2024
@@ -22,7 +22,7 @@
<field name="objectId" column="object_id" length="64" nullable="true" />
<field name="objectClass" column="object_class" type="string" />
<field name="version" column="version" type="integer" />
<field name="data" column="data" type="array" />
Copy link
Member

Choose a reason for hiding this comment

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

Strange, that it worked before.

What do you think mentioning this change in changelog, if it's necessary?

@Wojdylak Wojdylak merged commit da3542e into Sylius:1.13 Mar 21, 2024
28 checks passed
@Wojdylak
Copy link
Member

Thank you, @TheMilek!

@TheMilek TheMilek deleted the SYL-2874 branch March 21, 2024 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). Maintenance CI configurations, READMEs, releases, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] Consider replacing DB DC2TYPE:array with json_array app wide
5 participants