Background:

My content module contains a big list of pointers to default content files (json), which I need to periodically update to reflect changes from Drupal UI. However, if anything gets deleted in Drupal, my content module is broken until I identify exactly what was deleted, as well as its dependencies, and purge it from my content module.

Here's my workflow:

- Create a content module for use with Default Content, including pointers to json files in info.yml
- Deploy module to a collaborate environment for content managers to create, update content
- Update content module using "drush dcem"

Observed behavior:

Drush command terminated abnormally due to an unrecoverable error.
Error: Call to a member function id() on a non-object in .../default_content/src/DefaultContentManager.php, line 293

DefaultContentManager::exportModuleContent() calls loadEntityByUuid(), but doesn't bother to check the return value. For a non-existent uuid, the return value is null. exportModuleContent() then calls $entity->id(), resulting in this fatal.

Proposed behavior:

Generally, a missing uuid shouldn't cause a systemic failure.
At the very least, it would be helpful to know what is the offending uuid, so that I can resolve my data issue.
At best, the rest of the import should proceed (maybe there's a flag to "stop on error")

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aaronbauman created an issue. See original summary.

AaronBauman’s picture

Status: Active » Needs review
FileSize
5.83 KB

In this patch:

changes to default_content.drush.inc

  • add option ignore-missing-uuid: if given, allow dcem export to proceed; do not halt entire export for a single missing UUID
  • add examples to dcem definition
  • in drush_default_content_export_module(): if ignore-missing-uuid option is given, print out drush warnings for any UUIDs which were not found.

changes to DefaultContentManager and DefaultContentManagerInterface

  • Add optional argument $ignore_missing = FALSE
  • Add return value check in core loop to respect $ignore_missing option
  • When $ignore_missing is FALSE, throw \InvalidArgumentException (instead of allowing PHP fatal) when an entity cannot be loaded for any UUID

changes to DefaultContentManagerIntegrationTest:

  • New test testModuleExportException to cover new exception throwing (for missing uuid) and new $ignore_missing option.
larowlan’s picture

Status: Needs review » Closed (works as designed)

Hi
Your use case sounds like you're using default_content for content deployment.
That is out of scope for this module.
It is designed for modules/install profiles to provide default content where either

  1. the module/profile needs some defined content to function; or
  2. the module/profile wants to demonstrate how something functions by providing sample content

Moving content between sites/environments is the domain of the deploy suite and entity pilot et al.
I don't want default content to become increasingly complex in an effort to use it for purposes beyond its original intent.
Thanks
Lee

AaronBauman’s picture

Thanks for the feedback.
Any other pointers on how to bundle content with an install profile?

Use case for Deploy or Entity Pilot seems like it's around juggling content between environments, but I want something self-contained. Default Content, i thought, was the best module for that use case.

Would you at least consider a more limited-scope patch which throws InvalidArgumentException, rather than fatal-erroring out?

AaronBauman’s picture

Status: Closed (works as designed) » Needs review
FileSize
2.57 KB

Here's the more limited patch with just the return-value checking / exception throwing

AaronBauman’s picture

andypost’s picture

I just faced with the same issue - need to update default content in install profile module

+++ b/tests/src/Kernel/DefaultContentManagerIntegrationTest.php
@@ -186,4 +186,27 @@ class DefaultContentManagerIntegrationTest extends KernelTestBase {
+    try {
+      $this->defaultContentManager->exportModuleContent('default_content_export_test');
+    }
+    catch (\InvalidArgumentException $e) {
+      ¶
+    }
+    $this->assertTrue($e instanceof \InvalidArgumentException);

better to change that to

try
  call
  assertFail
catch
  assertPass

The last submitted patch, 5: default_content-throw_exception_missing_uuids-2851559.patch, failed testing.

larowlan’s picture

So, you're letting content editors edit content for an install profile?
That is in scope, it just wasn't clear from the OP

andypost’s picture

Yep, I'm trying to use default content as "fixture" as addition to config_installer
This way simplifies dev workflow - no reason to use db dump from live/stage for local dev
I just need sometimes to update it)

AaronBauman’s picture

Here's a re-roll of #6, using @expectedException annotation instead of try/catch

Status: Needs review » Needs work
dawehner’s picture

+++ b/src/DefaultContentManager.php
@@ -290,6 +290,9 @@ class DefaultContentManager implements DefaultContentManagerInterface {
+          throw new \InvalidArgumentException(new FormattableMarkup('Entity @type with UUID @uuid does not exist', ['@type' => $entity_type, '@uuid' => $uuid]));

Nitpick|: Afaik we use sprintf for generating exception messages.

larowlan’s picture

+++ b/tests/src/Kernel/DefaultContentManagerIntegrationTest.php
@@ -186,4 +186,21 @@ class DefaultContentManagerIntegrationTest extends KernelTestBase {
+   * @expectedException \InvalidArgumentException

Can you use ->setExpectedException instead of the annotation - see https://thephp.cc/news/2016/02/questioning-phpunit-best-practices

Looking good, thanks.

AaronBauman’s picture

dawehner: the throw statement is nearly identical to 2 others in exportContentWithReferences. I opened a new issue for this #2870422: Exception messages should not use FormattableMarkup

larowlan: rerolled using setExpectedException instead of annotation.
no other changes.

Status: Needs review » Needs work
AaronBauman’s picture

- renamed IntegrationTest file to match class name
- switched argument message to use sprintf

interdiff w/ #15

Status: Needs review » Needs work
AaronBauman’s picture

Status: Needs work » Needs review
FileSize
2.22 KB
818 bytes

Forgot to update service reference in the test

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/src/ExporterInterface.php
@@ -41,6 +41,8 @@ interface ExporterInterface {
+   * @throws \InvalidArgumentException if any UUID is not found.

Nitpick: We usually move this explanation into the next line, but this can be fixed on commit of course :)

larowlan’s picture

Assigned: AaronBauman » andypost

For @andypost to eyeball

  • andypost committed 507733a on 8.x-1.x authored by aaronbauman
    Issue #2851559 by aaronbauman: exportModuleContent() dies on any non-...
andypost’s picture

Assigned: andypost » Unassigned
Status: Reviewed & tested by the community » Fixed
Related issues: +#2691085: Schedule alpha 9 release

Thanx for path! Commited for #2691085: Schedule alpha 9 release

Status: Fixed » Closed (fixed)

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