#2666392: Unable to revert third party settings via config import introduced a new optional $id argument to ConfigEntityTypeInterface::getPropertiesToExport(). Without supplying this argument, some entities now throw a SchemaIncompleteException.

We need to supply this argument to avoid prevent the exception from being thrown on Drupal core 8.6 and up.

CommentFileSizeAuthor
#15 2986899-15.patch1.72 KBgabesullice
#10 2986899-9-followup.patch901 byteswim leers
#7 interdiff-commit.txt554 bytesgabesullice
#5 2986899-5.patch1.77 KBgabesullice
#2 2986899-2.patch1.11 KBgabesullice

Comments

gabesullice created an issue. See original summary.

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 2986899-2.patch1.11 KB
gabesullice’s picture

gabesullice
Primary language English
commented
Related issues: +#2986901: Followup for #2666392: ConfigEntityTypeInterface::getPropertiesToExport no longer returns NULL, throws exception instead
gabesullice’s picture

gabesullice
Primary language English
commented

Ah, I misunderstood what ID needed to be passed to getPropertiesToExport. It's the ID of a particular config entity, not the ID of the entity type. Still playing around with this. At this point in the code, we don't have a particular entity available.

gabesullice’s picture

gabesullice
Primary language English
commented
Related issues: +#2483407: Allow configuration schema fallback in ConfigEntityType::getPropertiesToExport() to work without an ID
StatusFileSize
new 2986899-5.patch1.77 KB

Taking a different approach.

+++ b/src/ResourceType/ResourceTypeRepository.php
@@ -235,12 +235,13 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
       // @todo Uncomment the first line, remove everything else once https://www.drupal.org/project/drupal/issues/2483407 lands.
       // return array_keys($entity_type->getPropertiesToExport());

This can be removed because that issue turned out of be outdated by #2666392: Unable to revert third party settings via config import. It didn't resolve the fundamental need and in fact caused this issue.

  • gabesullice committed 6ff7619 on 8.x-2.x 
    Issue #2986899 by gabesullice: Drupal core compatibility:...
gabesullice’s picture

gabesullice
Primary language English
commented
Status: Needs review » Fixed
StatusFileSize
new interdiff-commit.txt554 bytes

Fixed the CS violation on commit.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Fixed » Active
Related issues:

I'm confused by #5. So I checked/read #2483407: Allow configuration schema fallback in ConfigEntityType::getPropertiesToExport() to work without an ID again, and found new replies (#34–#36), which were pointing to #2666392: Unable to revert third party settings via config import, and suddenly #5 made sense.

Still, I have concerns: #2483407-37: Allow configuration schema fallback in ConfigEntityType::getPropertiesToExport() to work without an ID + #2483407-38: Allow configuration schema fallback in ConfigEntityType::getPropertiesToExport() to work without an ID + #2666392-53: Unable to revert third party settings via config import.

+++ b/src/ResourceType/ResourceTypeRepository.php
@@ -233,14 +235,17 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
+      finally {

OMG I totally never realized we can finally use finally in Drupal 8! (Because it requires PHP >=5.5.9.)

gabesullice’s picture

gabesullice
Primary language English
commented

I think the committed fix is fine (for JSON API). Ideally we wouldn't need $base_properties at all, but as long as it's possible for the exception to be thrown or for NULL to be returned, as it was before, then we'll need that variable.

When/if the upstream issues you linked to are resolved, the exception simply will be thrown under fewer circumstances/not at all. IOW, the code will gracefully just start working better. The only thing that I can see for us to do in this issue is add a todo to monitor the upstream issue and remove the try/catch block if getPropertiesToExport stops throwing an exception.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new 2986899-9-followup.patch901 bytes

You're right.

Per #2483407-39: Allow configuration schema fallback in ConfigEntityType::getPropertiesToExport() to work without an ID, we need to bring back the deleted lines quoted below, because Alex Pott has confirmed that there's still more work to be done — previously he said it was a duplicate of #2666392, but he's changed his mind on that 👍.

-      // @todo Uncomment the first line, remove everything else once https://www.drupal.org/project/drupal/issues/2483407 lands.
-      // return array_keys($entity_type->getPropertiesToExport());
gabesullice’s picture

gabesullice
Primary language English
commented

Looks good, but not committing yet because once #2986901: Followup for #2666392: ConfigEntityTypeInterface::getPropertiesToExport no longer returns NULL, throws exception instead lands, we will be able to just revert this commit entirely.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Oh, HAH! Unexpected. But indeed even better! 👌

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Drupal core compatibility: SchemaIncompleteException for some config entity types » [PP-1] Drupal core compatibility: SchemaIncompleteException for some config entity types
Status: Needs review » Postponed

This is blocked on the #2986901: Followup for #2666392: ConfigEntityTypeInterface::getPropertiesToExport no longer returns NULL, throws exception instead issue in core now, which is currently RTBC. Looking forward to doing #11.

Thanks for being so diligent, @gabesullice!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: [PP-1] Drupal core compatibility: SchemaIncompleteException for some config entity types » Drupal core compatibility: SchemaIncompleteException for some config entity types
Status: Postponed » Active

#2986901: Followup for #2666392: ConfigEntityTypeInterface::getPropertiesToExport no longer returns NULL, throws exception instead landed, in both 8.7 and 8.6! The honor is yours, @gabesullice!

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 2986899-15.patch1.72 KB

Just gonna double check before I push the revert.

  • gabesullice committed 1eb8541 on 8.x-2.x 
    Revert "Issue #2986899 by gabesullice: Drupal core compatibility:...
gabesullice’s picture

gabesullice
Primary language English
commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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