Remove fieldDescription deprecation by VincentLanglet · Pull Request #1170 · sonata-project/SonataDoctrineORMAdminBundle

VincentLanglet · 2020-10-24T13:45:44Z

Subject

BC

Changelog

### Deprecated
- Instantiate a FieldDescription without passing the name as first argument

VincentLanglet · 2020-10-24T13:46:29Z

 {
-    public function __construct()
-    {
-        $this->parentAssociationMappings = [];


This is already the default value. And the __construct() override disallow to pass a name as argument.

VincentLanglet · 2020-10-24T13:47:47Z


        $this->type = $this->type ?: $associationMapping['type'];
        $this->mappingType = $this->mappingType ?: $associationMapping['type'];
+        // NEXT_MAJOR: Remove the next line.


The next line is uselss because the fieldName is supposed to be set to find the association mapping.
Plus in next major, the field name will be set with new FieldDescription('FieldName')

VincentLanglet · 2020-10-24T13:47:55Z


        $this->type = $this->type ?: $fieldMapping['type'];
        $this->mappingType = $this->mappingType ?: $fieldMapping['type'];
+        // NEXT_MAJOR: Remove the next line.


The next line is uselss because the fieldName is supposed to be set to find the association mapping.
Plus in next major, the field name will be set with new FieldDescription('FieldName')

VincentLanglet · 2020-10-24T13:54:04Z

What is wrong with static analysis ? @sonata-project/contributors

greg0ire · 2020-10-24T17:06:10Z

You have the click the button 😉

That issue is typical of circular dependencies, here is how we solve it at doctrine/persistence: https://github.com/doctrine/persistence/blob/6e297e5cd2d8aedda49a5bc0fa39c6682583dc67/.github/workflows/continuous-integration.yml#L8

Basically, I think you will have to let Composer know what version of sonata-project/doctrine-orm-admin-bundle otherwise No version set (parsed as 1.0.0).

@OskarStark I think the Github Action should fail when Composer fails, why is that not the case?

VincentLanglet · 2020-10-24T18:58:59Z

Basically, I think you will have to let Composer know what version of sonata-project/doctrine-orm-admin-bundle otherwise No version set (parsed as 1.0.0).

How can I do this ?

greg0ire · 2020-10-24T19:24:53Z

Use COMPOSER_ROOT_VERSION: "3.24.0". BTW, I just noticed that you tagged the latest version of this packages as 3.24 instead of 3.24.0… I'm not sure of the implications of that 😅

VincentLanglet · 2020-10-24T21:46:25Z

Use COMPOSER_ROOT_VERSION: "3.24.0".

This won't be a long term fix, or we will have to update the version every time.

And why the composer would work for test but not for quality assurance ?
Is there something wrong with the action @OskarStark ?

BTW, I just noticed that you tagged the latest version of this packages as 3.24 instead of 3.24.0… I'm not sure of the implications of that 😅

Should be fix.

greg0ire · 2020-10-24T21:59:34Z

And why the composer would work for test but not for quality assurance ?

Good question. Also, why does it fail only now and not before? I notice that the Composer command is not the same, but that's it. Note that Composer 2 has been released, maybe it has to do with that?

By setting this var you can specify the version of the root package, if it cannot be guessed from VCS info and is not present in composer.json.

This means the difference probably lies in that guess. For some reason, Composer does not have access at the branch name in the case of the QA workflow.

jordisala1991 · 2020-10-25T06:55:04Z

Does any of this helps? composer/composer#8579 (comment)

AFAIK it was only needed on block bundle 3.x because it has circular dependency with admin 3.x (and the provided solution is not working on every case, I copied it from the old travis config I will try to use somethinh from that comment).

jordisala1991 · 2020-10-25T07:46:20Z

Fixed here for blockbundle: sonata-project/SonataBlockBundle#773

VincentLanglet · 2020-10-25T07:57:03Z

Fixed here for blockbundle: sonata-project/SonataBlockBundle#773

I will wait for the dev-kit pr then :)

jordisala1991 · 2020-10-25T11:16:55Z

The problem is with this Pr: sonata-project/SonataAdminBundle#6438

I gave a reason to not add that conflict, and it also creates cyclic dependency, so IMho we should revert that part and add the requirement here

jordisala1991 · 2020-10-25T11:17:41Z

On the block bundle 3.x is needed because of another cyclic dependency (and we should avoid them), fixed in 4.x

VincentLanglet · 2020-10-25T14:29:16Z

The problem is with this Pr: sonata-project/SonataAdminBundle#6438

I gave a reason to not add that conflict, and it also creates cyclic dependency, so IMho we should revert that part and add the requirement here

It's not the same to add a requirement here or add a conflict to AdminBundle.

With a requirement, 3.25 orm cant be install without 3.78. But 3.78 can be install with 3.23 witch I wanted to avoid.

I dont understand why you call this a cyclic dependency. Its not because there is a conflict between admin and orm that admin needs orm.

jordisala1991 · 2020-10-25T15:10:11Z

The problem is with this Pr: sonata-project/SonataAdminBundle#6438

I gave a reason to not add that conflict, and it also creates cyclic dependency, so IMho we should revert that part and add the requirement here

It's not the same to add a requirement here or add a conflict to AdminBundle.

With a requirement, 3.25 orm cant be install without 3.78. But 3.78 can be install with 3.23 witch I wanted to avoid.

I dont understand why you call this a cyclic dependency. Its not because there is a conflict between admin and orm that admin needs orm.

Something is wrong if the admin bundle (which does not require orm or mongodb persistence , because it is the other way around) is relying on a specific behavior of a specific version of the persistence. We either have a behavioral bc break or I cant understand. Is the admin bundle who defines the interfaces and the expected behavior, and the persistence who implements those contracts...

VincentLanglet · 2020-10-25T15:30:17Z

Something is wrong if the admin bundle (which does not require orm or mongodb persistence , because it is the other way around) is relying on a specific behavior of a specific version of the persistence. We either have a behavioral bc break or I cant understand. Is the admin bundle who defines the interfaces and the expected behavior, and the persistence who implements those contracts...

To resume the situation:
Previously the admin bundle was always using a function A, but it was buggy.
Now, the admin is using a function A or B, depending on an option foo = true or false.

The right behavior is the following,

If foo = true, the function B should be use.
If foo = false, the function A should be use.

BUT, the persistence bundle was not passing the option, and the default value was true.
So, if you use an old version of persistence bundle, with the new version of admin bundle, you'll use function B even if the option foo is false (when you were using correctly the function A before).

In practice, the impact of the function A vs the function B is so minimal that I start to think that the best solution would be indeed to remove the conflict.

VincentLanglet · 2020-10-30T19:24:38Z

Ready to review now @sonata-project/contributors

phansys · 2020-10-31T14:46:47Z

@@ -35,6 +30,7 @@ public function setAssociationMapping($associationMapping)

        $this->type = $this->type ?: $associationMapping['type'];


Off topic: if these ternaries are intended to "catch" only null values (instead of other falsy values), we should use ?? instead.

VincentLanglet commented Oct 24, 2020

View reviewed changes

VincentLanglet requested a review from a team October 24, 2020 13:49

VincentLanglet closed this Oct 24, 2020

VincentLanglet reopened this Oct 24, 2020

VincentLanglet mentioned this pull request Oct 25, 2020

Fix block-bundle sonata-project/dev-kit#1206

Merged

VincentLanglet mentioned this pull request Oct 25, 2020

Remove conflict with persistence bundle sonata-project/SonataAdminBundle#6540

Merged

Remove fieldDescription deprecation

9a8053d

greg0ire added the patch label Oct 31, 2020

greg0ire approved these changes Oct 31, 2020

View reviewed changes

phansys reviewed Oct 31, 2020

View reviewed changes

phansys approved these changes Oct 31, 2020

View reviewed changes

VincentLanglet merged commit a1913e6 into sonata-project:3.x Oct 31, 2020

VincentLanglet deleted the fieldDescriptionDeprecation branch October 31, 2020 17:41

		@@ -35,6 +30,7 @@ public function setAssociationMapping($associationMapping)

		$this->type = $this->type ?: $associationMapping['type'];

Uh oh!

Conversation

VincentLanglet commented Oct 24, 2020

Subject

Changelog

Uh oh!

VincentLanglet Oct 24, 2020

Choose a reason for hiding this comment

Uh oh!

VincentLanglet Oct 24, 2020

Choose a reason for hiding this comment

Uh oh!

VincentLanglet Oct 24, 2020

Choose a reason for hiding this comment

Uh oh!

VincentLanglet commented Oct 24, 2020

Uh oh!

greg0ire commented Oct 24, 2020

Uh oh!

VincentLanglet commented Oct 24, 2020

Uh oh!

greg0ire commented Oct 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VincentLanglet commented Oct 24, 2020

Uh oh!

greg0ire commented Oct 24, 2020

Uh oh!

jordisala1991 commented Oct 25, 2020

Uh oh!

jordisala1991 commented Oct 25, 2020

Uh oh!

VincentLanglet commented Oct 25, 2020

Uh oh!

jordisala1991 commented Oct 25, 2020

Uh oh!

jordisala1991 commented Oct 25, 2020

Uh oh!

VincentLanglet commented Oct 25, 2020

Uh oh!

jordisala1991 commented Oct 25, 2020

Uh oh!

VincentLanglet commented Oct 25, 2020

Uh oh!

VincentLanglet commented Oct 30, 2020

Uh oh!

phansys Oct 31, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

greg0ire commented Oct 24, 2020 •

edited

Loading