Move getter from TaggedAdminInterface to AdminInterface #6713

VincentLanglet · 2020-12-16T22:18:35Z

Subject

I am targeting this branch, because BC. Because the TaggedAdminInterface is not released.
After the merge, I will edit the previous changelog.

Moving getListModes to AbstractTaggedAdmin is consistent with others changes and allow to restrict the visibility of the listModes property to private in nextMajor.

I also

Update the 3.x comments since I'm fixing conflicts
Fix some issues I find during the merge from 3.x into master

Changelog

### Added
- Added `AbstractTaggedAdmin::getListModes()`.
- Added `TaggedAdminInterface::getListModes()`.

franmomu · 2020-12-17T00:19:22Z

src/DependencyInjection/Admin/TaggedAdminInterface.php

+    /**
+     * @return array<string, array<string, mixed>>
+     */
+    public function getListModes();


why this method should be in this interface? listModes does not sound like should be in the tagged part.

It's tricky but it's related to the method showMosaicButton().

showMosaicButton is kinda the setter of the property listModes , and getListModes the getter.

src/Resources/public/vendor/admin-lte/yarn.lock

VincentLanglet · 2020-12-18T10:55:19Z

Please review @sonata-project/contributors. I'd like to merge 3.x into master after this.

Is it ok for you @franmomu ?

core23 · 2020-12-18T11:03:18Z

src/DependencyInjection/Admin/TaggedAdminInterface.php

+    /**
+     * @return array<string, array<string, mixed>>
+     */
+    public function getListModes();


Keep in mind, that this is a BC break.

The new interface is not released, so this is okay for now.

Yeah, I take advantage of the fact it's not released :)

franmomu · 2020-12-19T00:13:14Z

Is it ok for you @franmomu ?

To be honest, I'm not sure, it feels like mixing things here.

Reading #6614, if the idea of this interface is to add setters for what is configurable with sonata.admin tag, I guess getters should not go in the interface.

In this particular case I guess that showMosaicButton should maybe just store a boolean flag to see if it should show the mosaic button or not and maybe getListModes rely on that flag.

VincentLanglet · 2020-12-19T11:34:25Z

Is it ok for you @franmomu ?

To be honest, I'm not sure, it feels like mixing things here.

Reading #6614, if the idea of this interface is to add setters for what is configurable with sonata.admin tag, I guess getters should not go in the interface.

I added a commit. WDYT about it now ?

Setters are in AdminTaggedInterface
Getters are in AdminInterface
Both are in AbstractAdminTagged since it's easier this way and this allow to use private property.

In the next major we could work on removing the fact that the AdminInterface extends the AdminTaggedInterface.

VincentLanglet · 2020-12-19T12:03:13Z

The psalm error is weird:

It's working on 3.x but not my branch
I can fix it by commenting

public function getNewInstance()
    {
        $object = $this->getModelManager()->getModelInstance($this->getClass());

        // $this->appendParentObject($object);

        foreach ($this->getExtensions() as $extension) {
            $extension->alterNewInstance($this, $object);
        }

        return $object;
    }

I also can fix it by changing

final protected function appendParentObject(object $object): void

to

final protected function appendParentObject($object): void

VincentLanglet · 2020-12-20T18:34:19Z

I didn't solve this weird psalm issue, so I added psalm-suppress. This is ready @sonata-project/contributors

VincentLanglet · 2020-12-21T23:54:19Z

I finally find out how to solve my issue @sonata-project/contributors cc @franmomu

franmomu · 2020-12-22T00:51:02Z

I added a commit. WDYT about it now ?

Setters are in AdminTaggedInterface

Getters are in AdminInterface

Both are in AbstractAdminTagged since it's easier this way and this allow to use private property.

In the next major we could work on removing the fact that the AdminInterface extends the AdminTaggedInterface.

I don't think getters should be in AbstractAdminTagged, if they are in AdminInterface, AbstractAdmin is the one implementing that interface not AbstractAdminTagged.

VincentLanglet · 2020-12-22T10:29:58Z

I don't think getters should be in AbstractAdminTagged, if they are in AdminInterface, AbstractAdmin is the one implementing that interface not AbstractAdminTagged.

That's why I first put the getters in the TaggedAdminInterface.

I dont se any interest putting getters in AbstractAdmin and setters in AbstractTaggedAdmin.

Having both in AbstractTaggedAdmin allows to restrict the visibility of the class property to private.

It's true that to use the admin tag, only the setter are needed, so the interface should only have the setters. But our implementation can be provided with the getters too ; that's not forbidden.

VincentLanglet · 2020-12-28T15:53:12Z

Can we finish this @franmomu (cc @sonata-project/contributors)

Having both getter and setter in AbstractTaggedAdmin is easier in order to restrict the property visibility, which is something we want to do in the next Major. I think it's easier this way.

Then, I would say we can

Have getters in TaggedAdminInterface, because it's easier (what was currently done)
Have getters in AdminInterface, because it reflect more the reality (what is currently done in the PR, asked by @franmomu). But it doesn't forbid to have getter in the Abstract class.

For a longer explanation from slack:

There are multiple solutions. Recently I discovered that to use the admin tag, the admin class should have a specific constructor signature and some setters, which wasn’t in the AdminInterface.
So I created a TaggedAdminInterface and an AbstractTaggedAdmin with all of this, and move some existings setters and getters from the AdminInterface to the tagged one. But I forgot one !

In this PR, I wanted to move the last one, but then franmomu said that the TaggedAdminInterface should only have the setters and not the getters. This is theorically true, since the TaggedAdminInterface is only related to the admin tag. So I move back the getters from the TaggedAdminInterface to the AdminInterface ; but I still let the getters in the AbstractTaggedAdmin, which allow me to restrict the property to private.

I see 3 solutions:

Getters in AbstractAdmin and AdminInterface: Cannot restrict the visibility of property, but do not duplicate the phpdoc.

Getters in AbstractTaggedAdmin and AdminInterface: Both interface reflect the reality, the visibility of the AbstractTaggedAdmin can be private but the phpdoc have to be duplicated for getters (since AbstractTaggedAdmin only implements TaggedAdminInterface)

Getters in AbstractTaggedAdmin and TaggedAdminInterface: The visibility can be private, the phpdoc is not duplicated, the TaggedAdminInterface does not reflect the reality (since getters are not needed for the admin tag theorically) but is it important ? This interface is not really supposed to be implemented alone without the AdminInterface implementation.

I personnaly prefer the option 3, but I’m ok with the second one.

VincentLanglet · 2020-12-29T19:59:31Z

Since @OskarStark agree with me on the option 3, I updated the PR.

This just need your +1 @sonata-project/contributors ;)

franmomu · 2020-12-30T08:07:15Z

Having both getter and setter in AbstractTaggedAdmin is easier in order to restrict the property visibility, which is something we want to do in the next Major. I think it's easier this way.

Why do we want to restrict the property visibility of these properties in the abstract class? Having simple setters and getters for the property it's almost like if they were public.

VincentLanglet · 2020-12-30T10:01:54Z

Why do we want to restrict the property visibility of these properties in the abstract class? Having simple setters and getters for the property it's almost like if they were public.

It's not the same.
Getter/Setter:

will be final
will have typehint

franmomu · 2020-12-30T13:39:14Z

Why do we want to restrict the property visibility of these properties in the abstract class? Having simple setters and getters for the property it's almost like if they were public.

It's not the same.
Getter/Setter:

will be final

will have typehint

What's preventing having getters and setters with type declaration?

For the final modifier, to me AbstractTaggedAdmin looks more like a trait than an abstract class.

VincentLanglet · 2020-12-30T14:04:51Z

Why do we want to restrict the property visibility of these properties in the abstract class? Having simple setters and getters for the property it's almost like if they were public.

It's not the same.
Getter/Setter:

will be final

will have typehint

What's preventing having getters and setters with type declaration?

Setting a property which does not respect the type declaration.
Accessing the null property without setting

For the final modifier, to me AbstractTaggedAdmin looks more like a trait than an abstract class.

There was already a discussion about this.
Fact is we currently rely on the constructor signature in the compiler pass and a trait cannot have a constructor.
The taggedAdmin was the shortest way to respect all the condition of the compiler pass, but it was planned to refactor this for 5.0:

Stop relying on the constructor
Changing the class to a trait.

VincentLanglet · 2020-12-30T16:39:36Z

I created #6723 @franmomu

wbloszyk

Looks good for me. IMO You should not add release version in this PR but do it in seperate dediated PR.

franmomu · 2021-01-02T16:56:39Z

Fact is we currently rely on the constructor signature in the compiler pass and a trait cannot have a constructor.

Why not? https://3v4l.org/pJV5D

I'm not sure if these getters should be final, I was going to try to use a Service Locator to inject all these services and I have to override the getters in order to use the service locator.

VincentLanglet · 2021-01-02T18:39:11Z

Fact is we currently rely on the constructor signature in the compiler pass and a trait cannot have a constructor.

Why not? https://3v4l.org/pJV5D

Didnt know it worked. The first time I used Trait, all I see on internet was that constructor in Trait should never be done because it was a bad practice. I think it's because Trait should be seen like helpers and should have nothing to do with the class instantiation ; and overriding a trait constructor is maybe difficult.

I'm not sure if these getters should be final, I was going to try to use a Service Locator to inject all these services and I have to override the getters in order to use the service locator.

Firstly, I was following the whole "close API" strategy.

Secondly, If the admin tag is provided, the setters will be called by the compiler pass, so properties will be set and I don't see why it would be needed to change the getters in this case. So final made sens to me.

Can you develop your idea with service locators ? Does it mean you won't use the "admin" tag ?

Move getListModes into TaggedAdminInterface

c6246c9

VincentLanglet requested review from a team and franmomu December 16, 2020 22:18

VincentLanglet mentioned this pull request Dec 16, 2020

3.x into Master #6712

Merged

VincentLanglet force-pushed the listMode branch 2 times, most recently from ce73390 to d407edb Compare December 16, 2020 23:26

franmomu reviewed Dec 17, 2020

View reviewed changes

src/Resources/public/vendor/admin-lte/yarn.lock Outdated Show resolved Hide resolved

VincentLanglet added 3 commits December 17, 2020 10:57

Update 3.x comments

ec7dff2

Fix signature

ace22a9

Minor improvements

3091807

VincentLanglet force-pushed the listMode branch from d407edb to 3091807 Compare December 17, 2020 09:58

VincentLanglet requested a review from franmomu December 17, 2020 09:59

OskarStark added the minor label Dec 17, 2020

core23 reviewed Dec 18, 2020

View reviewed changes

core23 previously approved these changes Dec 18, 2020

View reviewed changes

VincentLanglet requested a review from a team December 18, 2020 22:05

VincentLanglet dismissed core23’s stale review via 8d37e38 December 19, 2020 11:28

VincentLanglet force-pushed the listMode branch 2 times, most recently from 0544967 to 1c08c17 Compare December 19, 2020 11:29

VincentLanglet mentioned this pull request Dec 19, 2020

Psalm is sometimes inferring the wrong type for no reason. vimeo/psalm#4868

Closed

VincentLanglet closed this Dec 20, 2020

VincentLanglet reopened this Dec 20, 2020

VincentLanglet force-pushed the listMode branch from e741383 to baf97b3 Compare December 20, 2020 17:54

VincentLanglet changed the title ~~Move getListModes into TaggedAdminInterface~~ Move getter from TaggedAdminInterface to AdminInterface Dec 20, 2020

franmomu mentioned this pull request Dec 21, 2020

Add generics to ModelManagerInterface #6721

Closed

VincentLanglet force-pushed the listMode branch 4 times, most recently from 72582cb to e919db5 Compare December 21, 2020 23:46

VincentLanglet added this to the 4.0 milestone Dec 23, 2020

Remove useless template

34f10d6

VincentLanglet force-pushed the listMode branch from e919db5 to 34f10d6 Compare December 29, 2020 19:04

Fix psalm issue

139d6c3

VincentLanglet requested review from OskarStark, phansys and jordisala1991 December 30, 2020 15:12

wbloszyk approved these changes Dec 31, 2020

View reviewed changes

VincentLanglet merged commit 5f8d438 into sonata-project:3.x Jan 1, 2021

VincentLanglet deleted the listMode branch January 1, 2021 23:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move getter from TaggedAdminInterface to AdminInterface #6713

Move getter from TaggedAdminInterface to AdminInterface #6713

VincentLanglet commented Dec 16, 2020 •

edited

Loading

franmomu Dec 17, 2020 •

edited

Loading

VincentLanglet Dec 17, 2020

VincentLanglet commented Dec 18, 2020

core23 Dec 18, 2020

VincentLanglet Dec 18, 2020

franmomu commented Dec 19, 2020

VincentLanglet commented Dec 19, 2020

VincentLanglet commented Dec 19, 2020 •

edited

Loading

VincentLanglet commented Dec 20, 2020

VincentLanglet commented Dec 21, 2020

franmomu commented Dec 22, 2020

VincentLanglet commented Dec 22, 2020

VincentLanglet commented Dec 28, 2020 •

edited

Loading

VincentLanglet commented Dec 29, 2020

franmomu commented Dec 30, 2020

VincentLanglet commented Dec 30, 2020

franmomu commented Dec 30, 2020

VincentLanglet commented Dec 30, 2020

VincentLanglet commented Dec 30, 2020

wbloszyk left a comment

franmomu commented Jan 2, 2021

VincentLanglet commented Jan 2, 2021

Move getter from TaggedAdminInterface to AdminInterface #6713

Move getter from TaggedAdminInterface to AdminInterface #6713

Conversation

VincentLanglet commented Dec 16, 2020 • edited Loading

Subject

Changelog

franmomu Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

VincentLanglet Dec 17, 2020

Choose a reason for hiding this comment

VincentLanglet commented Dec 18, 2020

core23 Dec 18, 2020

Choose a reason for hiding this comment

VincentLanglet Dec 18, 2020

Choose a reason for hiding this comment

franmomu commented Dec 19, 2020

VincentLanglet commented Dec 19, 2020

VincentLanglet commented Dec 19, 2020 • edited Loading

VincentLanglet commented Dec 20, 2020

VincentLanglet commented Dec 21, 2020

franmomu commented Dec 22, 2020

VincentLanglet commented Dec 22, 2020

VincentLanglet commented Dec 28, 2020 • edited Loading

VincentLanglet commented Dec 29, 2020

franmomu commented Dec 30, 2020

VincentLanglet commented Dec 30, 2020

franmomu commented Dec 30, 2020

VincentLanglet commented Dec 30, 2020

VincentLanglet commented Dec 30, 2020

wbloszyk left a comment

Choose a reason for hiding this comment

franmomu commented Jan 2, 2021

VincentLanglet commented Jan 2, 2021

VincentLanglet commented Dec 16, 2020 •

edited

Loading

franmomu Dec 17, 2020 •

edited

Loading

VincentLanglet commented Dec 19, 2020 •

edited

Loading

VincentLanglet commented Dec 28, 2020 •

edited

Loading