I have found a bug.

If snippet_manager configuration is imported on clean site, it will crash with error:

[error]  Error: Call to a member function id() on boolean in filter_default_format() (line 212 of /var/www/html/web/core/modules/filter/filter.module) #0 /var/www/html/web/modules/contrib/snippet_manager/src/Entity/Snippet.php(276): filter_default_format()

The problem is that module is use text filters from core filter module. But module itself has no dependency to it. So if config imported before filter is installed via config, or even will be disabled on site, this will throw error.

I see two ways to fix it:
1. Add to module dependency to filter module. But it's seems not working, I don't know why but I tried and this not solve the problem.
2. This way is works and tested. We need to add filter module as dependency for configuration

dependencies: { }

replace by 

dependencies:
  module:
    - filter

I interested, what way do you prefer and if second one, how to implement it. I found that dependencies can be set by variable plugins, but not for snippet itself which uses filter for template.

CommentFileSizeAuthor
#18 2952920-18-snippet_manager_variable_collection.patch21.37 KBchi
#8 2952920-3.patch2.48 KBniklan
#7 2952920-2.patch1.13 KBniklan
#4 2952920-1.patch325 bytesniklan

Comments

Niklan created an issue. See original summary.

niklan’s picture

niklan
Primary language Russian
Location Russia, Perm
commented
Issue summary: View changes
chi’s picture

chi commented

We should add a dependency on core filter module.

niklan’s picture

niklan
Primary language Russian
Location Russia, Perm
commented
StatusFileSize
new 2952920-1.patch325 bytes

I have added dependency for filter module, but it not working. Still need to add filter module depencency to config. I don't know why. I looked at \Core\Config\Entity\ConfigEntityInterface::calculateDependencies() and \Drupal\Core\Config\Entity\ConfigEntityBase::addDependency() there is a comment:

A config entity is always dependent on its provider. There is no need to explicitly declare the dependency. An explicit dependency on Core, which provides some plugins, is also not needed.

So we don't need to add module itself to config dependency. But there is the problem. I don't exactly understand how, but during the import, seems like snippet_manager is not enabled at this moment, and so, filter is no enabled too, which cause this error.

chi’s picture

chi commented 
+++ b/snippet_manager.info.yml
@@ -4,4 +4,5 @@ description: Manages code snippets on a site.
+  - filter

Should be drupal:filter.

chi’s picture

chi commented

like snippet_manager is not enabled at this moment

Per the backtrace the error happens when Drupal\snippet_manager\Entity\Snippet object is being instantiated.

niklan’s picture

niklan
Primary language Russian
Location Russia, Perm
commented
StatusFileSize
new 2952920-2.patch1.13 KB

This patch is temporary fix for problem to get it working, before better solution will be found.

niklan’s picture

niklan
Primary language Russian
Location Russia, Perm
commented
StatusFileSize
new 2952920-3.patch2.48 KB

This patch adds dependency to configurations of filters used in formattable areas. Also it adds dependencies to modules which defines plugin variables used in snippet.

chi’s picture

chi commented

Seems like Drupal core does not follow this approach. Note that in example below there is no dependency on basic_html format though it is used to format default value.

$ drush cget field.field.node.article.body
uuid: c59c691f-f59e-4d25-a1f2-2075291ec32e
langcode: en
status: true
dependencies:
  config:
    - field.storage.node.body
    - node.type.article
  module:
    - text
_core:
  default_config_hash: Ay3b2hq42cpQTFB_lNu8S2ZxuVIY6-dlBsc7vLeJ-YY
id: node.article.body
field_name: body
entity_type: node
bundle: article
label: Body
description: ''
required: false
translatable: true
default_value:
  -
    summary: ''
    value: "<p>Test</p>\r\n"
    format: basic_html
default_value_callback: ''
settings:
  display_summary: true
field_type: text_with_summary

Another example is a View that has Text area plugin configured.

chi’s picture

chi commented

config imported before filter is installed via config

[error] Error: Call to a member function id() on boolean in filter_default_format() (line 212 of /var/www/html/web/core/modules/filter/filter.module) #0 /var/www/html/web/modules/contrib/snippet_manager/src/Entity/Snippet.php(276): filter_default_format()

Per the error message you provided the error happened in filter_default_format() function which belongs to the filter module. This means that the filter module was enabled at that moment.

I guess it fails because the system has not available filter formats at all.

  • Chi committed 0608b96 on 8.x-1.x 
    Issue #2952920 by Niklan: Optimized default format check.
niklan’s picture

niklan
Primary language Russian
Location Russia, Perm
commented

I guess it fails because the system has not available filter formats at all.

Seems like this is the core problem of this issue, I didn't think about that.

I will test later patch from commit above. Looks like this is also fix it.

But what's about dependencies for modules which define custom SnippetManagerVariablePlugin in patch 3 from #8?

And last commit missing dependency drupal:filter for module itself.

chi’s picture

chi commented
Assigned: Unassigned » chi
Status: Active » Needs review

The last submitted patch, 4: 2952920-1.patch, failed testing. View results

The last submitted patch, 7: 2952920-2.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 8: 2952920-3.patch, failed testing. View results

niklan’s picture

niklan
Primary language Russian
Location Russia, Perm
commented

RTBC for 0608b96. This is fix main error for issue.

chi’s picture

chi commented
StatusFileSize
new 2952920-18-snippet_manager_variable_collection.patch21.37 KB

In attached patch snippet variables are bundled into lazy plugin collection. For such collections core configuration system magically applies some operation when the configuration entity is being processed. For instance it supports "bubbling" dependencies from the collected plugins. While the patch works well (at least tests passed) I am inclined not to commit it. We don't really need a lazy loader for snippet variable plugins and the level of complexity does not match the benefits we gain by using this plugin collection.

And besides, the patch changes the structure of snippet configuration entity. So that upgrade path is needed.

  • Chi committed 26531c8 on 8.x-1.x authored by Niklan 
    Issue #2952920 by Niklan Added dependency calculation.
  • Chi committed a5355c5 on 8.x-1.x
    Issue #2952920: Calculate variable plugin dependencies.
chi’s picture

chi commented
Assigned: chi » Unassigned
Status: Needs work » Fixed

I've just added a calculation of dependencies including filter formats. Thanks.

Status: Fixed » Closed (fixed)

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