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")
Comment | File | Size | Author |
---|---|---|---|
#19 | interdiff.txt | 818 bytes | AaronBauman |
#19 | default_content-throw_exception_missing_uuids-2851559-19.patch | 2.22 KB | AaronBauman |
| |||
#17 | interdiff.txt | 1006 bytes | AaronBauman |
#17 | default_content-throw_exception_missing_uuids-2851559-17.patch | 2.22 KB | AaronBauman |
#15 | default_content-throw_exception_missing_uuids-2851559-15.patch | 2.14 KB | AaronBauman |
Comments
Comment #2
AaronBaumanIn this patch:
changes to default_content.drush.inc
ignore-missing-uuid
: if given, allow dcem export to proceed; do not halt entire export for a single missing UUIDdrush_default_content_export_module()
: ifignore-missing-uuid
option is given, print out drush warnings for any UUIDs which were not found.changes to
DefaultContentManager
andDefaultContentManagerInterface
$ignore_missing = FALSE
$ignore_missing
option$ignore_missing
is FALSE, throw\InvalidArgumentException
(instead of allowing PHP fatal) when an entity cannot be loaded for any UUIDchanges to
DefaultContentManagerIntegrationTest
:testModuleExportException
to cover new exception throwing (for missing uuid) and new$ignore_missing
option.Comment #3
larowlanHi
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
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
Comment #4
AaronBaumanThanks 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?
Comment #5
AaronBaumanHere's the more limited patch with just the return-value checking / exception throwing
Comment #6
AaronBaumannow with updated test
Comment #7
andypostI just faced with the same issue - need to update default content in install profile module
better to change that to
Comment #9
larowlanSo, you're letting content editors edit content for an install profile?
That is in scope, it just wasn't clear from the OP
Comment #10
andypostYep, 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)
Comment #11
AaronBaumanHere's a re-roll of #6, using @expectedException annotation instead of try/catch
Comment #13
dawehnerNitpick|: Afaik we use
sprintf
for generating exception messages.Comment #14
larowlanCan you use
->setExpectedException
instead of the annotation - see https://thephp.cc/news/2016/02/questioning-phpunit-best-practicesLooking good, thanks.
Comment #15
AaronBaumandawehner: the
throw
statement is nearly identical to 2 others inexportContentWithReferences
. I opened a new issue for this #2870422: Exception messages should not use FormattableMarkuplarowlan: rerolled using setExpectedException instead of annotation.
no other changes.
Comment #17
AaronBauman- renamed IntegrationTest file to match class name
- switched argument message to use sprintf
interdiff w/ #15
Comment #19
AaronBaumanForgot to update service reference in the test
Comment #20
dawehnerNitpick: We usually move this explanation into the next line, but this can be fixed on commit of course :)
Comment #21
larowlanFor @andypost to eyeball
Comment #23
andypostThanx for path! Commited for #2691085: Schedule alpha 9 release