Still on Drupal 7? Security support for Drupal 7 ended on 5 January 2025. Please visit our Drupal 7 End of Life resources page to review all of your options.Problem/Motivation
When calling \Drupal\eca\Plugin\ECA\Modeller\ModellerInterface::save, we receive a boolean return value which indicates whether the model needs to be reloaded or not, i.e. if it was new or if the title got changed. As part of that method we also call \Drupal\eca\Service\Modellers::saveModel which actually saves the config entities. Potential errors are caught and written to the logger.
Other tasks like e.g. cloning a model, goes through similar steps but errors get displayed in the UI.
Proposed resolution
We should remove the try-catch from the saveModel method and handle exceptions in all other places that use that method directly or indirectly, depending on the context, and handle the feedback (message on screen or in log) appropriately.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | error_message.png | 38.9 KB | rkoller |
Issue fork eca-3275457
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
jurgenhaasMR !130 is now handling exceptions consistently for all cases where ECA models get saved, e.g. save through ajax, import through form or Drush, cloning, enabling and disabling, etc. Each of those require specific handling of output to the user.
Side note: when going through all this, I thought we should probably have extracted all these functions into a submodule called
ECA UIwhich could be enabled separately, just like views ui or fields ui. ECA as such could operate without any UI component perfectly, once the models are configured and deployed. Should we still go for that before 1.0? Doesn't seem to be a big deal, but may make another big difference to the architecture.Comment #4
rkollerI've applied the patch and tested it against the model the initial report was about. the error message isnt several screens long anymore. if you can confirm if those error message informations would be the only relevant and correct then i would consider the issue fixed:
about your side note. i cant say anything about if it would be a reasonable step to create a separate ECA UI submodule, but if that is a significant change i wouldn't introduce something like that post 1.0 so the architecture would change another time right after a stable release .0 release.
i leave the issue status at needs review so others could chime in about the side note question and in regards if the display issue is fixed.
Comment #5
mxhGood idea, we could then move all list builders, controllers, form objects and maybe even permissions into that sub-module. The ECA config entity may then be extended by this module via
hook_entity_type_build, similar as field_ui does it.Comment #6
jurgenhaasMerged latest changes from 1.0.x-dev here too for better review.
Comment #7
mxhCode looks good, haven't tested this one manually though as @rkoller obviously already did it :)
The only thing that raised a question mark was why we check for \LogicException and EntityStorageException but nothing else. But since no other type of error was detected I guess it's good to go.
Comment #8
mxhWe could address
ECA UIwithin its own issue.Comment #10
jurgenhaasAlso, thanks so much for this one @mxh and @rkoller
The LogicException hit me a couple of times when there have been schema violations or missing plugins somewhere in the Drupal installation. They are not documented, I just discovered them by debugging the code.
And yes, let's address
ECA UIin a separate issue.