Follow-up to #2862083: [META] Improve paragraphs summary

Problem/Motivation

We currently support a few field types in getSummary. Modules that implement other field types could use a hook to handle their types.

Proposed resolution

Let's first see how much does current solution already cover and if we see that there are more use cases that we can't handle in Paragraphs, add the hook.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#46 2868252-paragraph-summary-alter-46.patch6.62 KBgmercer
#42 2868252-paragraph-summary-alter-42.patch6.52 KBAndrew Robinson
#38 2868252-paragraph-summary-alter-38.patch6.52 KBa.dmitriiev
#38 interdiff_33_38.txt7.99 KBa.dmitriiev
#33 interdiff.txt352 bytesLal_
#33 paragraphs-summary-2868252-33.patch3.65 KBLal_
#31 paragraphs-summary-events-2868252-29.patch3.51 KBsam.spinoy@gmail.com
#29 paragraphs-summary-events-2868252-29.patch0 bytessam.spinoy@gmail.com
#28 i2868252-28.patch7.12 KBattiks
#27 i2868252-27.patch7.1 KBattiks
#22 paragraphs-summary-events-2868252-21.patch5.05 KBdpacassi
#21 interdiff_19-21.txt4.77 KBdpacassi
#19 paragraphs-summary-events-2868252-19.patch3.56 KBsam.spinoy@gmail.com
#18 paragraphs-summary-events-2868252-18.patch3.15 KBsam.spinoy@gmail.com
#14 paragraphs-summary-events-2868252-14.patch1.13 KBkevineinarsson
#11 paragraphs-summary-events-2868252-11.patch5.79 KBsam.spinoy@gmail.com
#10 paragraphs-summary-events-2868252-10.patch5.03 KBthelmer
#7 consider_adding_a_hook-2868252-7.patch1.17 KBGinovski
#4 paragraphs-summary-events-2868252-3.patch5.31 KBthelmer
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Primsi created an issue. See original summary.

Primsi’s picture

thelmer’s picture

I have created a patch where the getSummary dispatches two different events :

ParagraphSummaryExcludeFieldsEvent
Dispatched in the beginning of getSummary, subscribers can add fields excluded from the summary

ParagraphSummaryAlterEvent
Dispatched after data is collected from the fields, subscriber can prepend/append strings to the summary array

The subscriber could do something like this:

/**
 * @file
 * Contains \Drupal\mymodule\ModuleEventSubscriber.
 */
namespace Drupal\mymodule\EventSubscriber;
use Drupal\paragraphs\Event\ParagraphSummaryAlterEvent;
use Drupal\paragraphs\Event\ParagraphSummaryExcludeFieldsEvent;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;

/**
 * Class ModuleEventSubscriber.
 *
 * @package Drupal\mymodule
 */
class ModuleEventSubscriber implements EventSubscriberInterface {
  /**
   * {@inheritdoc}
   */
  public static function getSubscribedEvents() {
    $events[ParagraphSummaryAlterEvent::ALTER][] = array('onAlterSummary', 800);
    $events[ParagraphSummaryExcludeFieldsEvent::EXCLUDEFIELDS][] = array('onExcludeFields', 800);
    return $events;
  }

  /**
   * Subscriber Callback for the event.
   * @param ParagraphSummaryAlterEvent $event
   */
  public function onAlterSummary(ParagraphSummaryAlterEvent $event) {
    $event->prependTextToSummary('#' . $event->getParagraph()->Id());
   }

   /**
   * Subscriber Callback for the event.
   * @param ParagraphSummaryExcludeFieldsEvent $event
   */
  public function onExcludeFields(ParagraphSummaryExcludeFieldsEvent $event) {
    $event->excludeFields(['field_button_color', 'field_button_text']);
  }
}
thelmer’s picture

NB: there are some uncommented code for list_string fields.
Currently only the first value (not the label) are being outputted.
I will work on some code so all labels are outputted.

Berdir’s picture

Status: Postponed » Needs review
Ginovski’s picture

Priority: Minor » Normal
Status: Needs review » Needs work

Needs reroll, summary method is changed from #2852001: Consider container multivalue children for closed summary

Ginovski’s picture

Status: Needs work » Needs review
FileSize
1.17 KB

Different approach with the alter hook.

miro_dietiker’s picture

Status: Needs review » Needs work

IMHO this doesn't help too much.

We are updating here the summary that then is concatenated.

We have other issues that will extend the summary with two icon areas per summary. Those icons should be similarly alterable by the hook. The collapsed plain text summary is just one summary component.

miro_dietiker’s picture

thelmer’s picture

sam.spinoy@gmail.com’s picture

miro_dietiker’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 11: paragraphs-summary-events-2868252-11.patch, failed testing. View results

kevineinarsson’s picture

Status: Needs work » Needs review
FileSize
1.13 KB

Rerolled #7 to keep the ball rolling. Why should this be event driven? Sorry if it's an ignorant question, but I don't see why this shouldn't just be an alter

rwohleb’s picture

I think the alter hook would be a lot more useful if the $summary array was keyed by field id. As it is now we can add summaries, but it doesn't give us an easy way to selectively strip certain fields.

Additionally, having the $options array that is passed to the getSummary method would be useful. It would allow any additions to respect the options.

Berdir’s picture

That's definitely true but also challenging, considering that we generate the summary recursively in some cases. We'd need to either switch to a nested structure or introduce a solution for having unique flat keys. Not sure yet.

rwohleb’s picture

After working with this over the weekend, I'd definitely vote for the nested structure. I was simplifying/adjusting output to be more user-friendly for editors and it would have made parts of it a lot easier. The order of fields is particularly an issue.

Additionally, the forced implode(',', $summary) just didn't work for what I was trying to do. I eventually had to combine everything the way I wanted and inserted it as a single entry returned by the hook to avoid that. It got me thinking that there should probably be a new template for summaries, and the implode behavior could be handled there.

sam.spinoy@gmail.com’s picture

sam.spinoy@gmail.com’s picture

The last submitted patch, 18: paragraphs-summary-events-2868252-18.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

dpacassi’s picture

FileSize
4.77 KB

The patch provided in #19 doesn't fire the exclude fields event.
I've fixed this and also a few coding standard issues.

dpacassi’s picture

Berdir’s picture

Status: Needs review » Needs work

The internal structure of the summary is still in flux and evolving, we've now changed it to an array of content and behavior items, so we're a step closer. But to make any sense of it being alterable, we also need named keys and a stable structure.

People who used this patch might also want to check if the new summary template and corresponding preprocess helps them do what they want to do.

miro_dietiker’s picture

Yeah, we should put those keys asap.

What could still add one last(?) change in the summary structure is #2932406: Display field image thumbnail in the paragraph summary

miro_dietiker’s picture

Promoting this to major so we will look into it when we review the Roadmap. :-)

johnchque’s picture

Priority: Normal » Major
attiks’s picture

Status: Needs work » Needs review
FileSize
7.1 KB

New approach, calls the alter inside the loop and passes field name and field type, so implementing a subscriber will be easier

Example subscriber https://www.drupal.org/project/viewsreference/issues/3137849#comment-136...

attiks’s picture

Better event name

sam.spinoy@gmail.com’s picture

I've reworked the patch from #19 for the latest paragraphs version.

sam.spinoy@gmail.com’s picture

sam.spinoy@gmail.com’s picture

something went wrong with the patch there, here is the correct version

Lal_’s picture

Status: Needs review » Needs work

Alter also means completely changing the summary, currently there aren't any way to remove the existing summary data and replace it with something else. Lets add that also..

Lal_’s picture

Status: Needs work » Needs review
FileSize
3.65 KB
352 bytes
bvoynick’s picture

Comparing patch 28 vs patch 29, we seem to have lost the actual trigger for the fields exclusion event.

LRoels’s picture

Status: Needs review » Reviewed & tested by the community

Patch #33 seems to work and include the needed functionalities.

Just tested this on my project and worked without issues.
Tested against 8.x-1.12.

pingevt’s picture

Patch in #33 seems to work for me against 8.x-1.13.

My use case is to give more information around a "From Library" paragraph. For now just prepending the Paragraph bundle of the item in the Library.

a.dmitriiev’s picture

I wonder why the summary is only limited to content in patch #33, there is also behaviors and maybe potentially more array properties of summary. I think it is better to allow modifying the summary array completely if needed, this is the main purpose of alter.

a.dmitriiev’s picture

Here is modified patch to allow alterations to any parts of summary

HitchShock’s picture

Both patches (#38 and #33) work for me on my project. +1 RTBC

Andrew Robinson’s picture

I believe these patches fail in Drupal 10 sites with the error:

Uncaught Error: Class "Symfony\Component\EventDispatcher\Event" not found in modules/contrib/paragraphs/src/Event/ParagraphSummaryAlterEvent.php:4

According to vendor/symfony/event-dispatcher/CHANGELOG.md, in Symfony 5.x
The `Event` class has been removed in favor of `Symfony\Contracts\EventDispatcher\Event`.

Based on this I think the use statement at the top of src/Event/ParagraphSummaryAlterEvent.php should be updated
from
use Symfony\Component\EventDispatcher\Event;
to
use Symfony\Contracts\EventDispatcher\Event;

for sites that are based on Symfony 5 or above.

a.dmitriiev’s picture

@vladigor@gmail.com In patch #38 this class is not used. It uses the class Drupal\Component\EventDispatcher\Event according to change record: https://www.drupal.org/node/3159012

Andrew Robinson’s picture

Thanks @a.dmitriiev.
I've switched over to patch #38 on my Drupal 10.1.0 site using Paragraphs 1.15.0.
When I try to edit a page that contains paragraphs I get the following error:

TypeError: Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::dispatch(): Argument #1 ($event) must be of type object, string given, called in /app/docroot/modules/contrib/paragraphs/src/Entity/Paragraph.php on line 468 in Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch() (line 89 of core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php).

It appears that in Drupal 10.1 the event_dispatcher service uses the dispatch function from ContainerAwareEventDispatcher.php, which expects $event as the first parameter.
Ref. https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Even...

I've attached an updated version of patch #38 that switches around the parameters in the two calls to the dispatch function.

LRoels’s picture

Status: Reviewed & tested by the community » Needs review

Changing the status to needs review for now. Seems wrong to keep it on RTBC.

LiamPower’s picture

#42 is working for me with Drupal 10.1.5 and Paragraphs 8.x-1.16

a.dmitriiev’s picture

Status: Needs review » Reviewed & tested by the community

#42 is working for me as well for Drupal 10

gmercer’s picture

Re-rolled #42 patch to check for empty of summary[$type] on appendTextToSummary() and prependTextToSummary() in src/Event/ParagraphSummaryAlterEvent.php