Fix getId/id return type for entities #208

diimpp · 2021-02-19T11:31:28Z

Changes are non BC, modification of mapping does not result in any new SQL statements

Fixes #206

diimpp · 2021-02-19T11:43:25Z

src/Resources/config/doctrine/BillingData.orm.xml

@@ -1,19 +1,23 @@
 <?xml version="1.0" encoding="UTF-8"?>

-<doctrine-mapping xmlns="http://doctrine-project.org/schemas/orm/doctrine-mapping" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://doctrine-project.org/schemas/orm/doctrine-mapping http://doctrine-project.org/schemas/orm/doctrine-mapping.xsd">
+<doctrine-mapping xmlns="http://doctrine-project.org/schemas/orm/doctrine-mapping"


I've set this "header" as it's done at sylius/sylius

diimpp · 2021-09-27T12:07:22Z

src/Entity/InvoiceShopBillingData.php

@@ -45,64 +44,64 @@ public function getCompany(): ?string
        return $this->company;
    }

-    public function getTaxId(): ?string
+    public function setCompany(?string $company): void


Methods re-arranged in order of class variables declaration/doctrine entities declaration

diimpp · 2021-09-27T12:09:03Z

@lchrusciel review please

GSadee · 2021-11-30T12:02:55Z

src/Resources/config/doctrine/BillingData.orm.xml

+        <field name="firstName" column="first_name" type="string" />
+        <field name="lastName" column="last_name" type="string" />
+        <field name="company" column="company" type="string" nullable="true" />
+        <field name="street" column="street" type="string" />
+        <field name="city" column="city" type="string" />
+        <field name="postcode" column="postcode" type="string" />
+        <field name="countryCode" column="country_code" type="string" />
+        <field name="provinceCode" column="province_code" type="string" nullable="true" />
+        <field name="provinceName" column="province_name" type="string" nullable="true" />


Why did you want to normalize mappings by adding these default values that are not necessary? The same applies to changes in other mappings

How come column type is not a necessary field? Why rely on doctrine to have expected defaults instead of setting correct type explicitly?

There is difference in how source code is written at App\ level and Sylius\Sylius level.
This is as much documentation as correct way to write entity mappings.

Besides, this is the way all sylius/sylius mappings are done.
https://github.com/Sylius/Sylius/blob/master/src/Sylius/Bundle/CoreBundle/Resources/config/doctrine/model/Order.orm.xml

The default value of type is string, so IMO there is no need to add it here, as they weren't defined before, especially in the PR with the title Fix getId/id return type for entities, if you would like to change that, it should be done in the separate PR

Doctrine mappings and entity fields types are quite related. Which issue will be solved by moving this to separate PR?

Please ask for 3rd opinion within the core team. Personally I don't see what there to discuss, sylius/sylius defines explicit doctrine mappings and that's how it should be done.

If you have different opinion on the matter -- please open RFC at sylius/sylius to remove string type definition from mappings.

@diimpp you've opened a PR with fixing getId return types, but you've also refactored other methods and the corresponding mapping, this refactoring should be done in a separate PR.

And about type field, in Sylius sometimes we define explicitly type and sometimes we don't, so there is no one proper way based on Sylius repository 😃 For me, and also for doctrine documentation, type is an optional field, so there is no need to define it, especially if they weren't configured before.

And fyi, I've already asked @Zales0123 and @lchrusciel for other opinions 😃

GSadee · 2021-11-30T12:08:41Z

src/Entity/LineItem.php

@@ -64,12 +63,12 @@ public function __construct(
        $this->taxRate = $taxRate;
    }

-    public function getId()
+    public function getId(): ?int


I would be for leaving it as it was before, without return type to keep id more customizable and consistent with all Sylius resources. I know that our mapping is configured for using integers, but it is relatively easy to change the strategy to use for example UUIDs instead of integers.

I would agree that getId(): ?int is opinionated, but not because somebody might want to make that UUID, but because sylius resource bundle doesn't define type-hint.

Though this is concrete implementation that you're marking final. In such case it's totally alright to make it int and not to support some doctrine hacks for 3rd party.

Did you see, that BillingData id is already int in class, but ShopBillingData is not? This is inconsistency and my PR is solving this as well.

Should Invoice's protected string $id became @mixed as well and constructor's string type-hint removed, because somebody might want to change DB id to int?

GSadee · 2021-11-30T12:26:06Z

@diimpp Sorry for that, especially that the PR have been opened for a long time, but I'm closing it as these are the changes that we don't want to have in this plugin. Of course, feel free to share your thoughts about that if you want to.

diimpp · 2021-12-03T08:52:15Z

@GSadee same change to the RefundPlugin was accepted. Sylius/RefundPlugin#199
There are tests and many other fixes besides getId(): ?int.

How come you're closing this?

diimpp · 2021-12-03T08:55:19Z

@GSadee don't you see related bug report as well? #206

diimpp · 2021-12-03T09:15:17Z

I've rebased PR to the current master, I can remove getId(): ?int typehint in favor of getId(), but it means no getId(): string as well.

diimpp · 2021-12-20T11:33:09Z

@GSadee @Zales0123 @CoderMaggie hey guys, closure of this PR needs to be re discussed.

There are bugs, that this PR closes and very similar PR for RefundPlugin was merged. I've answered above for all raised points.

Sylius/RefundPlugin#199

Zales0123 · 2022-02-25T14:01:06Z

Hello @diimpp! I once again, after @GSadee, apologise for such a long response time. Going straight to the problems about the review here.

I would definitely bump the idea of extracting some changes from this PR. It's named Fix getId/id return type for entities but there are definitely more things done here. You doubted in this comment that extracting these changes would make any difference, but the conversation around them clearly shows, that the opinions and comments touch different areas of application (e.g. configuration vs implementation) - which can be a clue that these are separate changes and should be processed separately :)
I would resign from changing the return type of getId() methods. You're both (@diimpp and @GSadee) right - this is not defined in the ResourceBundle so should not be here as well (first premise) and because of that can be configured to be e.g. UUID rather than int and we should not block it (second premise) 💃
I'm having a big dilemma with default types in mapping configuration 💥 On the one hand, indeed in most of the places in Sylius we define them. I don't know is it because when Sylius started to be developed it was not yet clear in Doctrine? Or maybe our approach was different? 🤷‍♂️ No clue. On the other hand, we trust Symfony and Doctrine as our main dependencies and have a great test suite, that would protect us from any differences if those happens :) So I would personally be for not defining them... but I have no strong opinion about that. Maybe @lchrusciel has some opinion?

Cheers and sorry one more time for such a delay in answer 🖖

diimpp · 2022-03-07T15:56:55Z

@Zales0123 Hi, thank you for update.

Please feel free to re-use any commits or changes from this PR.
You need to decide on the strategy, there are all three options already in the codebase with no type hints, with string and int type hints. There is open bug with overly strict int type hint as well, which is open for more than a year now. (Side point, both of you suggesting somebody might want to change ID field type to string/UUID, but think about this from regular developer view point -- you install bundle from packagist and you're going to change ID mappings of 3rd party code without and public contract or official support? Obviously not, acceptable solution is to introduce publicId property with any data type and not to touch original mappings, so this case is not important for consideration)
Explicit doctrine mappings are form of documentation as well as already selected approach for sylius/sylius:1. Look at this from end-developer view point, who just does another task for the acme company and doesn't care for doctrine or sylius defaults -- those explicit mappings helps such developer.
I remember there was some weird field on some entity either in this plugin or refund plugin, where I couldn't detect field content from the codebase and mappings were missing. I ended up experimenting on mysql5.7,8 to see which defaults would be real. What about postgresql in this case? There is just too much uncertainty for plugin's end user.

diimpp requested a review from a team as a code owner February 19, 2021 11:31

diimpp commented Feb 19, 2021

View reviewed changes

Dmitri Perunov added 3 commits September 27, 2021 17:01

Normalize doctrine fields mappings

9e08b54

Add spec for InvoiceSequence and InvoiceShopBillingData

db6c77d

Fix getId/id return type for entities

a4fbe03

diimpp force-pushed the bugfix/entity_id_return_type branch from d657369 to a4fbe03 Compare September 27, 2021 12:02

diimpp commented Sep 27, 2021

View reviewed changes

GSadee reviewed Nov 30, 2021

View reviewed changes

GSadee closed this Nov 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix getId/id return type for entities #208

Fix getId/id return type for entities #208

diimpp commented Feb 19, 2021 •

edited

Loading

diimpp Feb 19, 2021

diimpp Sep 27, 2021

diimpp commented Sep 27, 2021

GSadee Nov 30, 2021

diimpp Dec 3, 2021

GSadee Dec 20, 2021

diimpp Dec 20, 2021

GSadee Dec 20, 2021 •

edited

Loading

GSadee Dec 20, 2021

GSadee Nov 30, 2021

diimpp Dec 3, 2021 •

edited

Loading

diimpp Dec 3, 2021

diimpp Dec 3, 2021

GSadee commented Nov 30, 2021

diimpp commented Dec 3, 2021

diimpp commented Dec 3, 2021

diimpp commented Dec 3, 2021

diimpp commented Dec 20, 2021 •

edited

Loading

Zales0123 commented Feb 25, 2022

diimpp commented Mar 7, 2022

Fix getId/id return type for entities #208

Fix getId/id return type for entities #208

Conversation

diimpp commented Feb 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diimpp commented Sep 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GSadee Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diimpp Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GSadee commented Nov 30, 2021

diimpp commented Dec 3, 2021

diimpp commented Dec 3, 2021

diimpp commented Dec 3, 2021

diimpp commented Dec 20, 2021 • edited Loading

Zales0123 commented Feb 25, 2022

diimpp commented Mar 7, 2022

diimpp commented Feb 19, 2021 •

edited

Loading

GSadee Dec 20, 2021 •

edited

Loading

diimpp Dec 3, 2021 •

edited

Loading

diimpp commented Dec 20, 2021 •

edited

Loading