Library currently allows to work around paragraph limitations.

We probably need to add validation to prevent that.

CommentFileSizeAuthor
#20 validate_library-2882390-20.patch7.84 KBprimsi
#20 interdiff-validate_library-2882390-20.patch.txt2.02 KBprimsi
#18 validate_library-2882390-18.patch7.58 KBmbovan
#17 validate_library-2882390-17.patch7.54 KBprimsi
#17 interdiff-validate_library-2882390-17.patch.txt1.57 KBprimsi
#16 validate_library-2882390-16.patch7.53 KBprimsi
#16 interdiff-validate_library-2882390-16.patch.txt6.65 KBprimsi
#14 validate_library-2882390-13.patch6.63 KBprimsi
#14 interdiff-validate_library-2882390-13.patch.txt2.42 KBprimsi
#12 validate_library-2882390-12.patch7.09 KBprimsi
#12 interdiff-validate_library-2882390-12.patch.txt2.42 KBprimsi
#10 validate_library-2882390-10.patch7.74 KBprimsi
#10 interdiff-validate_library-2882390-10.patch.txt568 bytesprimsi
#6 validate_library-2882390-6.patch7.19 KBprimsi

Comments

Berdir created an issue. See original summary.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

I guess you mean that the "add to library" action is not displayed?
A validation is a bit late.

I proposed that we should have a checkbox if a paragraph type is allowed to be put into the library.
Are we now only looking at selection plugin settings?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

no, I'm not talking about add to library.

I'm talking about adding a library paragraph and then selecting one through auto-complete (or entity browser, or ..).

Not sure if there's an easy way to already limit it in the autocomplete/selection plugin, that's why I said validation.

maybe we have access to the field then it might be possible. needs to be investigated.

primsi’s picture

primsi commented
Assigned: Unassigned » primsi
primsi’s picture

primsi commented

As per discussion, we will define a third party setting per paragraph type, that states, that this paragraph type can be converted into a library item. We will then check for that while displaying the button and possible creating the library entity.

primsi’s picture

primsi commented
Status: Active » Postponed
StatusFileSize
new validate_library-2882390-6.patch7.19 KB

This relies on #2884095: Add paragraphs entity to hook_paragraph_widget_dropbutton_alter. Also a bit of scope creep with the use statements.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Postponed » Needs review

Committed the other. Now let's see.

Status: Needs review » Needs work

The last submitted patch, 6: validate_library-2882390-6.patch, failed testing. View results

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

:-) Schema errors for paragraphs.paragraphs_type.text with the following errors: paragraphs.paragraphs_type.text:third_party_settings.paragraphs_library.disallow_library_conversion missing schema

primsi’s picture

primsi commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-validate_library-2882390-10.patch.txt568 bytes
new validate_library-2882390-10.patch7.74 KB

Let's try again.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
  1. +++ b/modules/paragraphs_library/config/schema/paragraphs_library.schema.yml
@@ -0,0 +1,7 @@
+    disallow_library_conversion:
+      type: integer
+      label: 'Disallow conversion to paragraph library item'

    this means it is an opt-out and not an opt-in, not sure what makes more sense. 

  2. +++ b/modules/paragraphs_library/paragraphs_library.module
@@ -21,22 +22,30 @@ function paragraphs_library_paragraph_widget_dropbutton_alter(&$links, &$context
+    /** @var ParagraphsTypeInterface $paragraphs_type */
+    $paragraphs_type = \Drupal::entityTypeManager()->getStorage('paragraphs_type')->load($bundle);

    I think we have getParagraphType() on $paragraph for this, problem is just that ParagraphsInterface is missing most of the methods it should have.

Other than that, this looks pretty good. Only problem is that this is not what I created this issue for, it was the other side of this problem, the part where you can select whatever kind of library item you want and store it, even though the target paragraph type wouldn't be allowed in this context. We either need a new issue for what the issue was about or a new issue for what the patch is about :)

primsi’s picture

primsi commented
StatusFileSize
new interdiff-validate_library-2882390-12.patch.txt2.42 KB
new validate_library-2882390-12.patch7.09 KB

this means it is an opt-out and not an opt-in, not sure what makes more sense.

I first did it the other way around, but then it felt to me that this way to be more natural. I thought that if one installs paragraphs_library, having the ability to convert paragraphs to library without additional steps is what I would expect. Both approaches are valid though.

We either need a new issue for what the issue was about or a new issue for what the patch is about :)

:) Then given that we already derailed this issue my vote goes for a to rename this issue and add another one.

Status: Needs review » Needs work

The last submitted patch, 12: validate_library-2882390-12.patch, failed testing. View results

primsi’s picture

primsi commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-validate_library-2882390-13.patch.txt2.42 KB
new validate_library-2882390-13.patch6.63 KB

Did not rebase.

primsi’s picture

primsi commented

Follow up: #2884387: Do in fact validate library paragraphs

primsi’s picture

primsi commented
StatusFileSize
new interdiff-validate_library-2882390-16.patch.txt6.65 KB
new validate_library-2882390-16.patch7.53 KB

As per discussion, require per type allow conversion.

primsi’s picture

primsi commented
StatusFileSize
new interdiff-validate_library-2882390-17.patch.txt1.57 KB
new validate_library-2882390-17.patch7.54 KB

Fixed schema type.

mbovan’s picture

mbovan commented
StatusFileSize
new validate_library-2882390-18.patch7.58 KB

Rerolled.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work 
+++ b/modules/paragraphs_library/paragraphs_library.module
@@ -134,3 +140,32 @@ function _paragraphs_library_count_usage(array $usage_data) {
+  $form['allow_library_conversion'] = [

Don't show this checkbox on from_library type.

primsi’s picture

primsi commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-validate_library-2882390-20.patch.txt2.02 KB
new validate_library-2882390-20.patch7.84 KB

This applies now only for type "from_library". Not sure if there is a better way to do it.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community 
+++ b/modules/paragraphs_library/config/schema/paragraphs_library.schema.yml
@@ -0,0 +1,7 @@
+  label: 'Per-paragraph type third party settings'

I think the label should more mention the library to be valuable. But at least in a test module i have seen this label skipped on this level.

Other than that i think we should get this in.

  • Primsi committed 94e51fd on 8.x-1.x 
    Issue #2882390 by Primsi, mbovan, miro_dietiker, Berdir: Validate...
primsi’s picture

primsi commented
Status: Reviewed & tested by the community » Fixed

Corrected the label and committed.

Status: Fixed » Closed (fixed)

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