Support paragraph type icon config deployment

Or we could keep it in the module static location and use that as a fallback.
And use the UUID based reference that user can use through the UI as an override to it.

Then, import doesn't need to process anything at all.

Hm, reverting relation changes...

The title is misleading. This is not a default icon that is a global fallback.
This is about an icon that is shipped as a default by a module if the paragraph type is imported as configuration.

Right. That said, a default icon ison (issue) is something we need as well I think? But that part is easy.

Also confused about your #3, which does seem to be about a default icon.

Also not sure what you need from me, I already provided two possible ways to do this, one would code based to copy the files, the other approach is to use default_config, but that also needs file_entity or https://www.drupal.org/project/better_normalizers.

Yeah, technically, #3 is a "default icon", but some that is limited to a specific paragraph type.

I understood the "default icon" as a super global icon fallback, that doesn't make sense. If a specific paragraph has no icon, we just don't show any icon. But i'm open to have a config path based default fallback icon that simply points to a file (and could be overridden by the user).

Since i'm strongly against adding dependencies to make the icons work, we have two options left:
- My per-Paragraph-Type config path that is technically a per-paragraph default icon
- Your copy on install

Prior to implementation, we still need a decision. :-)

I'm not sure how a string based approach would work, how would the generic code know that the icon for paragraph demo text + image is in paragraphs_demo module, for example? or in the new paragraphs_something experimental thing that we're currently discussing? Without the per-module stream wrappers which are still not in core (which would result in module://paragraphs_demo/icons/text_image.png, which would be very technical).

It would either need to be relative to public files directory, so we still have to copy, or absolute, but then we need to hardcode the path to the module (at install, and moving it would break it)

After running in to this we have posted some thoughts to solve this in #2830016: Add a thumbnail/icon field to Paragraphs type.

It would be nice if you could do something like

langcode: en
status: true
id: full_width_image
label: 'Full Width Image'
icon: '<?xml version="1.0" encoding="UTF-8"?><svg width="280px" height="200px" viewBox="0 0 280 200" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"><g id="Page-1" stroke="none" stroke-width="1" fill="none" fill-rule="evenodd"><g id="Full-Width-Image"><rect id="Rectangle" stroke="#6E6E6E" stroke-width="10" x="5" y="5" width="270" height="190" rx="8"></rect><rect id="Rectangle-2" fill="#B9B9B9" x="24" y="24" width="230" height="150"></rect></g></g></svg>'
description: 'An image that takes up the full width of the container.'
behavior_plugins: {  }

Or something. I don't see any reason we would need to accept binary images. We could just limit it to SVG and allow either people to upload them (in which cause you would use icon_uuid and file system) or paste the SVG code somewhere.

Here we have a situation of dependency on content from the configuration. It is similar to the "broken block effect", which occurs with instances of missing custom blocks (see #2918149: "This block is broken or missing..." should only be shown to users that have access to do something about it). This ends in an inconsistency that should be solved in some way.

One approach to solve this is to store in configuration a default content (serialized) to be used when the referenced content is missing. The content is recreated automatically from the default value when required (on demand). That's what I did in the fixed block content module, that solves the broken block effect.

With this patch, a new property is added to the paragraph type that stores the default icon, encoded in Base64 under the data URL scheme (RFC 2397). Everything works transparently, the default icon value is updated when the paragraph type is saved and the file icon is recreated if not found (it was deleted or it is a new environment).

The patch also creates the paragraphs icon location (public://paragraphs_type_icon/) if not present, as in new environments.

The URL scheme also opens the door to a new approach without any dependency on files. This format can be embed in CSS background images and in HTML "img" tags.

Note that, due to limitations of converting MIME to file extension, the patch works safely only with the supported icon types: JPG, PNG or SVG.

Failed tests do not seem to be related to the last patch.

I wouldn't be so sure about that, daily tests are passing just fine: https://www.drupal.org/node/2130961/qa

1. +++ b/src/Entity/ParagraphsType.php
   @@ -102,6 +110,52 @@ class ParagraphsType extends ConfigEntityBundleBase implements ParagraphsTypeInt
   +        $new_icon->save();
   

   I really don't like patterns where a getter implicitly triggers a save due to some magic functionality.

   Again, can we not simply just do this on config import only?

2. +++ b/src/Entity/ParagraphsType.php
   @@ -181,25 +235,23 @@ class ParagraphsType extends ConfigEntityBundleBase implements ParagraphsTypeInt
   +  public function preSave(EntityStorageInterface $storage) {
   ...
   +      $this->icon_default = 'data:' . $icon_file->getMimeType() . ';base64,' . base64_encode(file_get_contents($icon_file->getFileUri()));
   

   Either set this only if the icon changed on save or maybe only on config export. This now slows down all config saves.

Tests failed because I forget the calling to parent::preSave when presaving, so dependencies wasn't calculated. In addressing this, I found that the calculateDependencies() method currently returns the dependencies, when it must returns the object itself. I opened a new issue for that (#2967635: Paragraphs type calculateDependencies returns wrong value).

However, presave is no longer needed since the default icon value is now set when and where the UUID currently does, that is, in the paragraphs type form. That solves some unnecessary processing when the type is saved, as pointed by Miro. Perhaps a new "setIcon" method could a be better way, but it implies an API change.

I do not like to do something else than read on getters too, but by its nature, content entities are dynamic and they can be updated or deleted in the runtime, so the icon file could be missing not only when importing configuration in a new environment. At the moment, I do not have a better solution than check that the required content exists when it is needed.

Storing and serving the icon data as an embed image rather than a file would solve this and some other weakness around the current implementation, but it is something to evaluate out of this issue.

This patch no longer succeeds since 8.x-1.3. I've updated the diff to apply properly and re-uploaded.

Am I understanding right that this issue is just for config provided by a module?

Is there a proper way or thought about how to "deploy" an icon that I uploaded to my local so that it can be pushed, with the image, to the repo or something like that?

I notice an uuid is exported, but what's the use of that? This is probably a general problem, but any current recommended workaround?

Thanks!

@hanoii, from my point of view, the right way to add an icon file is by adding it to the site assets. Icons are not really pieces of content, so shouldn't be managed as so. Drupal core does it in this way. As an example, the core shortcut module provides some icons at core/modules/shortcut/images, then it makes use of them as a background images at core/modules/shortcut/css/shortcut.icons.theme.css

This issue attempts to solve the use of content that should be part of config or file assets. IMHO, we should turn this into a new issue to make paragraphs to manage their icons as the core does.

shortcut isn't using icons for user-created configuration, so that can't be compared.

What can be compared is block config/content entities, which is actually (not) working just like this in core as well.

The only option is similar to what the patch is doing, exporting the raw data in the config. However, I think it would be better to do that specifically during config import/export and not on any save operation. Similar to \Drupal\field\FieldStorageConfigStorage::mapFromStorageRecords()

System icon files, like the mentioned shortcut icon:

1. Provided by technicians (site builder, programmer...) during the site construction
2. Not intended to be managed by content editors or final site users
3. They are part of the site assets, not content

Paragraphs icons:

1. Provided by technicians (site builder, programmer...) during the site construction
2. Not intended to be managed by content editors or final site users
3. They should be part of the site assets, not content

Currently, paragraphs type icons are set in two locations, a reference in config and the file icon itself as content. So we have a config dependency on content that the patch tries to solve it by moving the icon file content to config.

@hanoii refers to a general strategy to manage situations like this one, and core does it very well: icon file as part of the site assets and reference to it when needed. For site icons, a good place would be "sites/default/assets/icons" or "assets/icons".

Hi,

The patch in #22 does not applies anymore, because tests have been moved in #2753889: Convert ParagraphsTestBase and the easy-to-convert tests extending it to BrowserTestBase

Here is a re-rolled patch.

Uploaded fix for failed test and replaced some deprecated code

Next patch

Functional review: works amazing!
Technical review (some thoughts):

1. +++ b/src/Entity/ParagraphsType.php
   @@ -97,10 +108,57 @@ class ParagraphsType extends ConfigEntityBundleBase implements ParagraphsTypeInt
   +        && $icon_file_uri = $filesystem->saveData($icon_data, $icon_file_destination)) {
   

   I would assign $icon_file_uri outside of if. It is easy to miss this method call.

2. +++ b/src/Entity/ParagraphsType.php
   @@ -97,10 +108,57 @@ class ParagraphsType extends ConfigEntityBundleBase implements ParagraphsTypeInt
   +        if ($old_icon = $this->getFileByUuid($this->icon_uuid)) {
   

   What if we re-save the existing file instead of deleting it?

3. +++ b/src/Entity/ParagraphsType.php
   @@ -97,10 +108,57 @@ class ParagraphsType extends ConfigEntityBundleBase implements ParagraphsTypeInt
   +    if ($this->icon_uuid && $icon = $this->getFileByUuid($this->icon_uuid) ?: $this->restoreDefaultIcon()) {
   

   Maybe I am the only one. But for me it is hard to read method calls and nested "ifs" inside the "if" condition.

Added some fixes related to comment #33

Issue is fixed

@KondratievaS, I guess the status you mean is RTBC ;-)

1. +++ b/src/Entity/ParagraphsType.php
   @@ -97,10 +107,59 @@ class ParagraphsType extends ConfigEntityBundleBase implements ParagraphsTypeInt
   +  protected function restoreDefaultIcon() {
   +    // Default icon data in RFC 2397 format ("data" URL scheme).
   +    if ($this->icon_default && $icon_data = fopen($this->icon_default, 'r')) {
   

   I did not now that data: suppots a media type, that's neat.

2. +++ b/src/Entity/ParagraphsType.php
   @@ -97,10 +107,59 @@ class ParagraphsType extends ConfigEntityBundleBase implements ParagraphsTypeInt
   +
   +        // Delete existent icon file if it exists.
   +        if ($old_icon = $this->getFileByUuid($this->icon_uuid)) {
   +          $old_icon->delete();
   +        }
   

   I'm not sure when this would be used?

   The only call to this method that I can see is when this very call fails, so in which case should it ever go in here?

+++ b/src/Form/ParagraphsTypeForm.php
@@ -168,12 +169,16 @@ class ParagraphsTypeForm extends EntityForm {
     $icon_file = $form_state->getValue(['icon_file', '0']);
-    // Set the file UUID to the paragraph configuration.
+    // Set the icon file UUID and default value to the paragraph configuration.
     if (!empty($icon_file) && $file = $this->entityTypeManager->getStorage('file')->load($icon_file)) {
       $paragraphs_type->set('icon_uuid', $file->uuid());
+      $paragraphs_type->set(
+        'icon_default',
+        'data:' . $file->getMimeType() . ';base64,' . base64_encode(file_get_contents($file->getFileUri())));
     }
     else {
       $paragraphs_type->set('icon_uuid', NULL);
+      $paragraphs_type->set('icon_default', NULL);
     }

Maybe setting/unsetting icon_default should be done in preSave(), to make sure that it never gets out of sync?

These patches were working great for us until recently when we tried using it in our install profile (which is using paragraphs and this patch).
It's strange because we've been using patch #22 for quite some time while performing clean installs but only recently we've noticed problems where an icon couldn't be moved from the "temporary" directory to it's destination, which is the icon directory in the files directory. Upon inspection we've noticed the upload directory didn't exist yet when trying to move the icon. This failure however led to an uncompleted installation which made it a big issue for us. We are not sure why this started happening but is it possible something changed in Drupal Core's install procedures which led to these recent failures?

Anyhow, by tinkering a tiny bit with the code in the patch (which modifies "src/Entity/ParagraphsType.php") we've had better results in our initial tests. What we've done is just move the "checking if the upload directory exists ($filesystem->prepareDirectory(...))" before the process of "saving the icon ($filesystem->saveData(...))".

diff --git a/src/Entity/ParagraphsType.php b/src/Entity/ParagraphsType.php
index 32b3025..9b815c0 100644
--- a/src/Entity/ParagraphsType.php
+++ b/src/Entity/ParagraphsType.php
@@ -127,10 +127,11 @@ class ParagraphsType extends ConfigEntityBundleBase implements ParagraphsTypeInt
       $filesystem = \Drupal::service('file_system');
       $icon_upload_path = ParagraphsTypeInterface::ICON_UPLOAD_LOCATION;
       $icon_file_destination = $icon_upload_path . $this->id() . '-default-icon.' . $icon_file_ext;
+      // Check the directory exists before writing data to it.
+      $filesystem->prepareDirectory($icon_upload_path, FileSystemInterface::CREATE_DIRECTORY | FileSystemInterface::MODIFY_PERMISSIONS);
       // Save the default icon file.
       $icon_file_uri = $filesystem->saveData($icon_data, $icon_file_destination);
-      if ($filesystem->prepareDirectory($icon_upload_path, FileSystemInterface::CREATE_DIRECTORY | FileSystemInterface::MODIFY_PERMISSIONS)
-        && $icon_file_uri) {
+      if ($icon_file_uri) {
         // Create the icon file entity.
         $icon_entity_values = [
           'uri' => $icon_file_uri,

We will test further to be sure. In the meantime please find a rewrite of the patch with our small modification in attachment.

Yes, that change definitely makes sense, the directory needs to be prepared before saving the data for it.

We've tested this in our project too and it seems to work nicely. Still room for improvements, but it's a good first step I think and has tests.

Sorry to resurrect the dead, but: how do I use this?
1.) I locally set icons for paragraphs.
2.) I export the config
3.) I push the config to staging system & import
4.) ???
5.) Profit. Icons showing up on stage.

I'm struggling at 4.)

There is no step 4. your exported config files should have a long base64 blob in them, that is the icon. if you import that, it should create the file and the icon should be there. Seems to work fine for us.

Got it. Thank you!