Problem/Motivation

abstract class ConfigEntityBase extends Entity implements ConfigEntityInterface, ThirdPartySettingsInterface {

This means that the interface itself does not implement ThirdPartySettingsInterface, so when you type hint on BlockInterface for example, you can't use those methods.

Proposed resolution

Move that to ConfigEntityInterface instead.

Remaining tasks

None

User interface changes

None

API changes

All modules with classes implementing ConfigEntityInterface will have ThirdPartySettings by default.
If a module has an interface extending ConfigEntityInterface and ThirdPartySettingsInterface, it will need to remove the ThirdPartySettingsInterface part.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task
Issue priority Not critical
Unfrozen changes Unfrozen because it only changes the type hinting of classes extending ConfigEntityInterface
Prioritized changes Follow up of #2361775: Third party settings dependencies cause config entity deletion
Disruption Non disruptive for contributed and custom modules because even if it's a change, they will only need to remove the ThirdPartySettingsInterface part of the classes extending ConfigEntityInterface
CommentFileSizeAuthor
#19 2425637-19.patch7.75 KBswentel
#15 2425637-15.patch7.84 KBpcambra
#15 interdiff-2425637-11-15.txt2.16 KBpcambra
#11 2425637-11.patch8.61 KBpcambra
#11 interdiff-2425637-8-11.txt2.35 KBpcambra
#8 2425637-8.patch6.99 KBpcambra
#8 interdiff-2425637-6-8.txt639 bytespcambra
#6 2425637-6.patch6.37 KBpcambra
#6 interdiff-2425637-4-6.txt5.22 KBpcambra
#4 2425637-4.patch1.14 KBpcambra
#1 2425637-1.patch1.51 KBpcambra
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pcambra’s picture

pcambra
he/him
Primary language Spanish
Location Asturies
CreditAttribution: pcambra commented
Status: Active » Needs review
FileSize
2425637-1.patch1.51 KB

Since we're here

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Thanks, looks like you included the use removal from the other issue in this patch, so it will probably conflict?

Status: Needs review » Needs work

The last submitted patch, 1: 2425637-1.patch, failed testing.

pcambra’s picture

pcambra
he/him
Primary language Spanish
Location Asturies
CreditAttribution: pcambra commented
Status: Needs work » Needs review
FileSize
2425637-4.patch1.14 KB

Oops, now should be better.

Status: Needs review » Needs work

The last submitted patch, 4: 2425637-4.patch, failed testing.

pcambra’s picture

pcambra
he/him
Primary language Spanish
Location Asturies
CreditAttribution: pcambra commented
Status: Needs work » Needs review
FileSize
interdiff-2425637-4-6.txt5.22 KB
2425637-6.patch6.37 KB

Replacing the usage in the rest of interfaces.

As discussed with berdir on IRC, this is a change that will require an action for some contribs.

Status: Needs review » Needs work

The last submitted patch, 6: 2425637-6.patch, failed testing.

pcambra’s picture

pcambra
he/him
Primary language Spanish
Location Asturies
CreditAttribution: pcambra commented
Status: Needs work » Needs review
FileSize
interdiff-2425637-6-8.txt639 bytes
2425637-8.patch6.99 KB

Had to convert Drupal\views_ui\ViewUI to abstract.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Needs work

That's not correct, that thing is not abstract. I guess we have to add the trait there or implement the methods properly (meaning, forward to $this->storage).

The last submitted patch, 8: 2425637-8.patch, failed testing.

pcambra’s picture

pcambra
he/him
Primary language Spanish
Location Asturies
CreditAttribution: pcambra commented
Status: Needs work » Needs review
FileSize
interdiff-2425637-8-11.txt2.35 KB
2425637-11.patch8.61 KB

The trait was removed in #2361775: Third party settings dependencies cause config entity deletion appartently so the only option would be to implement the methods, I grabbed them from ConfigEntityBase, not sure if that's what's expected here.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

I think you should forward all calls to $this->storage, see other methods there. ViewsUI is a wrapper class, that *acts* like an entity but it's not one actually.

Status: Needs review » Needs work

The last submitted patch, 11: 2425637-11.patch, failed testing.

pcambra’s picture

pcambra
he/him
Primary language Spanish
Location Asturies
CreditAttribution: pcambra commented

I think you should forward all calls to $this->storage, see other methods there. ViewsUI is a wrapper class, that *acts* like an entity but it's not one actually.

But storage refers to an Entity and not a Config entity in this case (I think) and those have no third party settings, I've tried to forward it to $this->storage->getExecutable or getViewExecutable which are config entities but I'm a little stuck on ViewUIObjectTest fails.

pcambra’s picture

pcambra
he/him
Primary language Spanish
Location Asturies
CreditAttribution: pcambra commented
Status: Needs work » Needs review
FileSize
interdiff-2425637-11-15.txt2.16 KB
2425637-15.patch7.84 KB

Everything makes much more sense now, thanks Berdir!

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Issue tags: +Needs issue summary update

thanks, looks good to me now.

Needs an issue summary update for the API change and beta evaluation...

pcambra’s picture

pcambra
he/him
Primary language Spanish
Location Asturies
CreditAttribution: pcambra commented
Issue summary: View changes
pcambra’s picture

pcambra
he/him
Primary language Spanish
Location Asturies
CreditAttribution: pcambra commented
Issue tags: -Needs issue summary update
swentel’s picture

swentel CreditAttribution: swentel commented
FileSize
2425637-19.patch7.75 KB

Needed a reroll

swentel@swenteldoos:/home/drupal/drupal-core$ git apply 2425637-15.patch
error: patch failed: core/modules/views_ui/src/ViewUI.php:1241
error: core/modules/views_ui/src/ViewUI.php: patch does not apply
bojanz’s picture

bojanz CreditAttribution: bojanz commented
Status: Needs review » Reviewed & tested by the community

The summary is up to date, the patch looks good. Let's go.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

Ah yes, I always wondered about that. Makes much more sense :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 27f0b0f and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 27f0b0f on 8.0.x 
    Issue #2425637 by pcambra, swentel: ConfigEntityInterface should extend...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.