Problem/Motivation

#2280961: (Views)FieldPluginBase::advancedRender() calls SafeMarkup::set() on a string that it doesn't know to be safe is supposed to fix all SafeMarkup::set() calls.

One of them is already solved in #2280961-25: (Views)FieldPluginBase::advancedRender() calls SafeMarkup::set() on a string that it doesn't know to be safe, the other ones are harder
but will certainly need a total orthogonal fix.

field/Field.php is using some items imploded with some string.

Proposed resolution

Let's get just that change in and work on the other problem.

This items imploded with some string can be replaced by a twig inline template.

Remaining tasks

User interface changes

API changes

On top of fixing the bug we introduce interfaces for field api handler:

  • \Drupal\views\Plugin\views\Field\FieldHandlerInterface: An interface for every views field
  • \Drupal\views\Plugin\views\field\MultiItemsFieldHandlerInterface: An interface for field handlers which renders multiple items per row

This is a pure API additional as currently everyone has to extend the FIeldPluginBase class anyway.
This API change makes it easier for people to write proper multi item field handlers.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task: Part of the existing SafeMarkup::set meta issue
Issue priority Inherits a critical from the parent issue
Disruption The API addition (interface) will not break any existing code, but just potential open up better ways.
CommentFileSizeAuthor
#16 2397727-16.patch27.55 KBjibran
#16 interdiff.txt1.42 KBjibran
#11 2397727-11.patch27.74 KBjibran
#11 interdiff.txt6.5 KBjibran
#7 2397727-7.patch32.45 KBjibran
#7 interdiff.txt19.41 KBjibran
#1 2397727-1.patch23.42 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
2397727-1.patch23.42 KB

.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Parent issue: » #2280961: (Views)FieldPluginBase::advancedRender() calls SafeMarkup::set() on a string that it doesn't know to be safe
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented 
  1. +++ b/core/modules/field/src/Plugin/views/field/Field.php
@@ -689,34 +690,25 @@ public function submitGroupByForm(&$form, FormStateInterface $form_state) {
+      return \Drupal::service('renderer')->render($build);

    injection 

  2. +++ b/core/modules/views/src/Plugin/views/field/FieldHandlerInterface.php
@@ -0,0 +1,195 @@
+   * Called to determine what to tell the clicksorter.
...
+   * Determine if this field is click sortable.

    This is not helpful at all. 

  3. +++ b/core/modules/views/src/Plugin/views/field/FieldHandlerInterface.php
@@ -0,0 +1,195 @@
+   * Get this field's label.

    Gets 

  4. +++ b/core/modules/views/src/Plugin/views/field/FieldHandlerInterface.php
@@ -0,0 +1,195 @@
+   * Return an HTML element based upon the field's element type.
...
+   * Return an HTML element for the label based upon the field's element type.
...
+   * Return an HTML element for the wrapper based upon the field's element type.
...
+   * Return the class of the field.
...
+   * Return the class of the field's label.
...
+   * Return the class of the field's wrapper.

    "Returns" and @param block missing. 

  5. +++ b/core/modules/views/src/Plugin/views/field/FieldHandlerInterface.php
@@ -0,0 +1,195 @@
+   * Provide a list of elements valid for field HTML.

    Provides 

  6. +++ b/core/modules/views/src/Plugin/views/field/FieldHandlerInterface.php
@@ -0,0 +1,195 @@
+   * Replace a value with tokens from the last field.

    Replaces and @param block missing. 

  7. +++ b/core/modules/views/src/Plugin/views/field/FieldHandlerInterface.php
@@ -0,0 +1,195 @@
+   * Get the value that's supposed to be rendered.

    Gets 

  8. +++ b/core/modules/views/src/Plugin/views/field/FieldHandlerInterface.php
@@ -0,0 +1,195 @@
+   * Run before any fields are rendered.

    Runs 

  9. +++ b/core/modules/views/src/Plugin/views/field/FieldHandlerInterface.php
@@ -0,0 +1,195 @@
+   * Render a field using advanced settings.

    Renders 

  10. +++ b/core/modules/views/src/Plugin/views/field/FieldHandlerInterface.php
@@ -0,0 +1,195 @@
+   * Perform an advanced text render for the item.

    Performs 

  11. +++ b/core/modules/views/src/Plugin/views/field/FieldHandlerInterface.php
@@ -0,0 +1,195 @@
+   * Trim the field down to the specified length.

    Trims 

  12. +++ b/core/modules/views/src/Plugin/views/field/FieldHandlerInterface.php
@@ -0,0 +1,195 @@
+   * Get the 'render' tokens to use for advanced rendering.

    Gets also @param block missing. 

  13. +++ b/core/modules/views/src/Plugin/views/field/FieldHandlerInterface.php
@@ -0,0 +1,195 @@
+   * Pass values to drupal_render() using $this->themeFunctions() as #theme.

    Passes 

  14. +++ b/core/modules/views/src/Plugin/views/field/MultiItemsFieldHandlerInterface.php
@@ -0,0 +1,53 @@
+   * Render all items in this field together.

    Renders 

  15. +++ b/core/modules/views/src/Plugin/views/field/PrerenderList.php
@@ -118,7 +114,7 @@ public function getItems($values) {
-    return method_exists($this, 'render_item');
+    return TRUE;

    What?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

@jibran
Thank you for your review, but please take in mind that we mostly move the existing public functions into an interface. Nitpicking them here is not that helpful.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Assigned: Unassigned » jibran

I know we are just moving the code around but we have to fix these doc issues so I'll try to reroll the patch.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Well, can you then please fix all other doc issues as well :P

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
FileSize
interdiff.txt19.41 KB
2397727-7.patch32.45 KB

Fixed the doc issues and renamed render_item to renderItem. Removed PrerenderList::allowAdvancedRender because now it is same as FieldPluginBase::allowAdvancedRender

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Assigned: jibran » Unassigned

@dawehner could you please review the doc changes?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

@jibran
Thank you for your hard work, but still I disagree with fixing all the docs here ... It is not the main point of this issue.

  1. +++ b/core/modules/field/src/Plugin/views/field/Field.php
@@ -870,10 +883,16 @@ function process_entity(EntityInterface $entity) {
+  /**
+   * {@inheritdoc}
+   */
+  function renderItem($count, $item) {

    Is there a reason why we need to fix all the @inheritdoc entries here? You know, fixing stuff is easy but reviewing all the crap is a different topic. Can't we fix that in a follow up? 

  2. +++ b/core/modules/user/src/Plugin/views/field/Permissions.php
@@ -113,7 +113,7 @@ public function preRender(&$values) {
-  function render_item($count, $item) {
+  function renderItem($count, $item) {

    Afaik renaming the function is totally unrelated to fix the issue ... it is an api change, no matter how you see it.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Assigned: Unassigned » jibran

I agree with @dawehner will fix both the points. Thanks for the review.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Assigned: jibran » Unassigned
FileSize
interdiff.txt6.5 KB
2397727-11.patch27.74 KB

Fixed #9

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Thank you!

Well, I'm fine with the patch as it is :)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Priority: Major » Critical
Issue tags: +VDC

Well, technically it is a subissue of an existing critical one

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs review » Reviewed & tested by the community

I think main thing here is done. I only fixed the doc so setting it to RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

The issue summary does not really cover why we are introducing the new interfaces. And the introduction of the new interfaces could be added to the API section.

A few minor nits. 

  1. +++ b/core/modules/field/src/Tests/Views/HandlerFieldFieldTest.php
@@ -7,6 +7,8 @@
+use Drupal\Component\Utility\String;
+use Drupal\Component\Utility\Xss;

    Not used 

  2. +++ b/core/modules/views/src/Plugin/views/field/FieldHandlerInterface.php
@@ -0,0 +1,266 @@
+   *
+   * @param bool $row_index
+   *   The index of current row.
+   */
+  public function tokenizeValue($value, $row_index = NULL);

    Missing param 

  3. +++ b/core/modules/views/src/Plugin/views/field/FieldHandlerInterface.php
@@ -0,0 +1,266 @@
+   * @return gst

    gst?

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.42 KB
2397727-16.patch27.55 KB

@dawehner can you please update the issue summary?

  1. Fixed.
  2. Fixed.
  3. FIxed. gst is an alias of `git status` so this is a repercussion of alt+tab gone wrong :D
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue summary: View changes

Adapted the IS to include some mentions about the new interfaces.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update +DX (Developer Experience)

So back to RTBC then.

This API change makes it easier for people to write proper multi item field handlers.

This qualify as an improved DX.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Assigned: Unassigned » alexpott

Back to Alex.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 041511a and pushed to 8.0.x. Thanks!

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes.

  • alexpott committed 041511a on 8.0.x 
    Issue #2397727 by jibran, dawehner: Remove the SafeMarkup::set() call in...

Status: Fixed » Closed (fixed)

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