In EntityNG text fields started using $item->processed instead of $item->safe_value. We need to make sure the prepareCache logic and the all the widgets/formatters use the same key. I'm not sure there are good reasons for changing the key, so I guess it should stay safe_value and safe_summary, i.e. renamed the computed EntityNG properties.

Then, the prepareCache() logic needs to be aligned such that the cached value can be set with NG API and it becomes cleared via onChange().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

Also, #1969728: Implement Field API "field types" as TypedData Plugins makes TextProcessed::setValue() to skip the read-only exception. Once it gets committed, this should be solved as part of this clean-up.

effulgentsia’s picture

Since we already have 'processed' and 'summary_processed' defined as our computed properties, I think we should stick with that, and remove all usages of 'safe_value' and 'safe_summary'. There's not that many of them.

Drupal\text\TextProcessed already implements the logic of text_sanitize(), so this patch should also remove that function. However, note that we need to add the nl2br() to the implementation of TextProcessed::getValue().

@fago: Can you roll the patch to do this? I'm a little lost as to what the correct way of setting (in order to cache) a computed property is.

effulgentsia’s picture

Title: Improve safe value handling and caching of text fields » Remove text_sanitize() since we have TextProcessed as an EntityNG computed property

Retitling

Berdir’s picture

Category: task » bug
Priority: Normal » Major
Issue tags: +Performance

The safe value caching is currently broken, seeing a lot of uncached calls to check_markup(), making that the third-slowest function (right after queries and cache prepareItem()) on the frontpage with 10 nodes.

Raising to major and adding Performance tag.

Berdir’s picture

Status: Active » Needs review
Issue tags: +API change
FileSize
10.6 KB
194.65 KB

This seems to be working pretty well, actually. And the performance gains are impressive, which has two reasons:

- safe_value worked. But getValue() also called processed->getValue(), multiple times, and that was completely uncached.
- TextProcessed also used to fetch references to the text and format properties and therefore instantiated them. That circumvents all the work that went into avoiding exactly that.

ab -n 100 -c 1 on frontpage with 10 nodes with a body
8.x:
Requests per second: 6.42 [#/sec] (mean)
Time per request: 155.842 [ms] (mean)

With patch:
Requests per second: 6.96 [#/sec] (mean)
Time per request: 143.652 [ms] (mean)

According to xhprof, 12% less function calls and 8.7% less wall time, which is quite similar to the values reported by ab.

processed_check_markup_xhprof.png

Just a few more crazy things like this and then there might still be hope that D8 will only be slow and not dead slow ;)

Status: Needs review » Needs work
Issue tags: -Performance, -API change, -Entity Field API

The last submitted patch, text-processed-caching-2026339-5.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Performance, +API change, +Entity Field API

The last submitted patch, text-processed-caching-2026339-5.patch, failed testing.

yched’s picture

  1. +++ b/core/modules/text/lib/Drupal/text/Plugin/field/field_type/TextItemBase.php
    @@ -83,10 +83,10 @@ public function prepareCache() {
    -          $this->set('safe_' . $source_property, text_sanitize($text_processing, $langcode, $itemBC, $source_property));
    

    There might still be code that tries to read the "old" properties (safe_body, safe_summary) ?
    Maybe this explains the fails ?

  2. +++ b/core/modules/text/lib/Drupal/text/TextProcessed.php
    @@ -21,18 +21,11 @@
    -  protected $text;
    -
    -  /**
    -   * The text format property.
    -   *
    -   * @var \Drupal\Core\TypedData\TypedDataInterface
    -   */
    -  protected $format;
    

    Should we really remove those ? the code still assigns $this->text / $this->format

  3. +++ b/core/modules/text/lib/Drupal/text/TextProcessed.php
    @@ -60,22 +53,28 @@ public function setContext($name = NULL, TypedDataInterface $parent = NULL) {
         $instance = field_info_instance($entity->entityType(), $field->getName(), $entity->bundle());
     
    -    if (!empty($instance['settings']['text_processing']) && $this->format->getValue()) {
    -      return check_markup($this->text->getValue(), $this->format->getValue(), $entity->language()->id);
    +    if (!empty($instance['settings']['text_processing']) && $this->parent->format) {
    

    While we're in there, could we switch to $field_definition = $field->getFieldDefinition();
    $field_definition->getFieldSetting('text_processing'); ?

  4. +++ b/core/modules/text/lib/Drupal/text/TextProcessed.php
    @@ -60,22 +53,28 @@ public function setContext($name = NULL, TypedDataInterface $parent = NULL) {
    +      $this->processed = check_markup($text, $this->parent->format, $entity->language()->id);
    

    According to #2061331-20: Add getEntity() helper in FieldInterface / FieldItemInterface and following, it's wrong to consider that the item language is the parent entity language. Current code is wrong too, but can we add a @todo on that issue here ? Would make it easier to later fix the places where we make that wrong assumption.

  5. +++ b/core/modules/text/lib/Drupal/text/TextProcessed.php
    @@ -83,9 +82,7 @@ public function getValue($langcode = NULL) {
    -      // @todo This is triggered from DatabaseStorageController::invokeFieldMethod()
    -      // in the case of case of non-NG entity types.
    -      // throw new ReadOnlyException('Unable to set a computed property.');
    +      $this->processed = $value;
    

    I'm not sure that this code branch in EntityStorageControllerBase::invokeFieldMethod() doesn't still run, but you probably know better than me :-)

+ doesn't apply anymore :-(

Berdir’s picture

Thanks.

1. I didn't find any when searching for it, My guess is more that it's about stale data, as processed is now kept on the class.

2. Oh, forgot to remove that part. The problem there is that get('text') and get('format') instantiate the property object, which completely defeats the purpose of that lazy-instantiation stuff. That wasn't properly updated when we introduced that. Experimented a bit with assigning the value but that just makes on change handling harder. They're not used anymore.

3. Was wondering about that, yes, will update it.

4. Will add a todo.

5. There are various cases where property objects get destroyed/set again, at least right now with the bc decorator. So we need to be able to set the processed value again or the caching doesn't actually work.

yched’s picture

5. Oh, right sorry, I misread the code. I though you had uncommented the exception back, but you removed it altogether. Fine :-)

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
5.82 KB
12.22 KB

Ok, this should behave better :)

- Switched to use NULL as "not yet calculated" instead of FALSE, because that's what gets set when you call $item->setValue() and processed is not explicitly defined.
- Removed unecessary setContext() stuff.
- Implemented onChange(), will need to update the DUBT tests to make sure that this works as expected.
- Implemented yched's feedback.

Status: Needs review » Needs work

The last submitted patch, text-processed-caching-2026339-12.patch, failed testing.

yched’s picture

Nitpicks:

  1. +++ b/core/modules/text/lib/Drupal/text/Plugin/field/field_type/TextItemBase.php
    @@ -71,22 +71,34 @@ public function isEmpty() {
         $text_processing = $this->getFieldSetting('text_processing');
         if (!$text_processing || filter_format_allowcache($this->get('format')->getValue())) {
    

    Nitpick: maybe we could inline the $text_processing var & switch to the "$this->format" syntax.
    (feel free to ignore me, those lines are not actually touched by the patch...)

  2. +++ b/core/modules/text/lib/Drupal/text/Plugin/field/field_type/TextItemBase.php
    @@ -71,22 +71,34 @@ public function isEmpty() {
    +  /**
    +   * @param $property_name
    +   */
    +  public function onChange($property_name) {
    

    weird phpdoc ?

  3. +++ b/core/modules/text/lib/Drupal/text/TextProcessed.php
    @@ -46,46 +39,38 @@ public function __construct(array $definition, $name = NULL, TypedDataInterface
    +    if ($field->getFieldDefinition()->getFieldSetting('text_processing') && $this->parent->format) {
    

    Ok, sorry, my bad - $field->getFieldSetting('text_processing') can be used as shorthand.

    And actually, $this->parent->getFieldSetting('text_processing') should work too, and then we don't need the $field and $entity vars (although maybe $item = $this->parent might make the code clearer)

Berdir’s picture

Actually, $field->getFieldSetting() was what I tried first, and it was a fatal error, that doesn't seem to exist? $this->parent->getFieldSetting() should work, yes, but I still need $entity for the broken language.

yched’s picture

Yes, sorry, got confused, Field / FieldItem::getFieldSetting() is an internal protected helper, so yup, here we need to go through getFieldDefinition().

Still the method could be streamlined like so:

  public function getValue($langcode = NULL) {
    if ($this->processed !== NULL) {
      return $this->processed;
    }

    $item = $this->parent;
    $text = $item->{($this->definition['settings']['text source'])};

    if ($item->getFieldDefinition()->getFieldSetting('text_processing') && $item->format) {
      // @todo: The entity language might not be the correct language to use,
      //   fix in https://drupal.org/node/2061331.
      $entity = $item->getParent()->getParent();
      $this->processed = check_markup($text, $item->format, $entity->language()->id);
    }
    else {
(...)
Berdir’s picture

Status: Needs work » Needs review
FileSize
3.52 KB
13.97 KB

Yes, that looks nice. Also fixed the tests and the docblock. I don't understand why this previously printed a \n at the end. Removed it from the tests for now unless someone knows if/why it's required.

yched’s picture

+++ b/core/modules/rdf/lib/Drupal/rdf/Tests/CommentAttributesTest.php
@@ -272,7 +272,7 @@ function _testBasicCommentRdfaMarkup($graph, $comment, $account = array()) {
-      'value' => $comment->comment_body->value . "\n",
+      'value' => $comment->comment_body->processed,

The previous code used the raw unsanitized 'body' value. Using the processed one instead means filters get applied, and I guess _filter_autop() adds a newline ?
I guess we could leave the test untouched (comment_body->value should still work, right ?). But the new code makes more sense (it's the processed version that should be in the HTML, the current test is just lucky that they kind of match) so this change looks fine.

+++ b/core/modules/rdf/lib/Drupal/rdf/Tests/StandardProfileTest.php
@@ -462,10 +462,7 @@ protected function assertRdfaNodeCommentProperties($graph) {
     $text = $this->articleComment->get('comment_body')->offsetGet(0)->get('value')->getValue();
     $expected_value = array(
       'type' => 'literal',
-      // There is an extra carriage return in the when parsing comments as
-      // output by Bartik, so it must be added to the expected value.
-      'value' => "$text
-",
+      'value' => $text,

That one seems weirder. Both the old and the new code use the unprocessed body value, and it shouldn't be different before and after the patch ?
+ for consistency with the other test, should this one move to ->processed as well ?

Other than that, the patch is RTBC for me, but it would be best if we got to the bottom of this...

Berdir’s picture

There's no filter format, so it runs through nl2br(check_plain($text)), both of them do, both of them failed with ->value and ->processed before the \n removal.

yched’s picture

No filter format ? There should be, comment_body is supposed to be filtered text.
Maybe the weirdness is related to what happens when a formatted text has format NULL ?

Berdir’s picture

Issue tags: +sprint

Yeah, I don't know, but I was seeing a lot of calls going through the no-format route, have to check again. Agreed that it's supposed to have a format and that could be the difference.

Tagging for rocketship focus

yched’s picture

So yes, the issue comes from the fact that those tests create comments without a format, and the patch changes how a "formatted text with a NULL format" is output.

- In HEAD, text_sanitize() does

if ($text_processing) {
  return check_markup($item[$column], $item['format']);
} 

so : check_markup($value, NULL), which applies filter_fallback_format(), that's 'plain_text' on a default install, which includes auto_p filter.
So: 'some value' -> '<p>some value</p>'

- with patch, TextProcessed::getValue() does

if ($text_processing && $item->format) {
  check_markup()
}
else {
  nl2br(check_plain())
}

So it's the second branch that runs if format is NULL: 'some value' -> 'some value'

Not sure which one is best in absolute, but we probably don't want to change the behavior of NULL format in this patch ?

If we stick to the current behavior and remove "&& $item->format" in TextProcessed::getValue(), then no need to change CommentAttributesTest & StandardProfileTest here.

Granted, those tests are buggy, they should:
- create comments with an actual format
- base their tests on the processed values rather than the raw value
But I'd say fixing them seems outside the scope of this specific patch.

Berdir’s picture

Nice find. agreed, let's keep the existing behavior. Removed that check. Let's see if those tests pass now.

There's a @todo in TextItemBase::applyDefaultValue() that currently defaults to NULL. We didn't have access to the field settings when we did that, but we should now use the field settings to do that correctly. Separate issue.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Sweet :-)
(+ awesome fixing a 10% perf regression...)

Yes, discussing NULL format and maybe filling in a default when none is provided in incoming values might be a good thing.

Berdir’s picture

Status: Reviewed & tested by the community » Needs work

Looks like either my changes or recent changes in HEAD broke the caching.

Berdir’s picture

Oh, actually it still works in HEAD but I was testing with the BC decorator removed, there it breaks.

We'll need to do this in the other issue:

diff --git a/core/lib/Drupal/Core/Entity/FieldableEntityStorageControllerBase.php b/core/lib/Drupal/Core/Entity/FieldableEntityStorageControllerBase.php
index ebec974..b546123 100644
--- a/core/lib/Drupal/Core/Entity/FieldableEntityStorageControllerBase.php
+++ b/core/lib/Drupal/Core/Entity/FieldableEntityStorageControllerBase.php
@@ -81,7 +81,7 @@ protected function loadFieldItems(array $entities, $age) {
           $data = array();
           $instances = field_info_instances($this->entityType, $entity->bundle());
           foreach ($instances as $instance) {
-            $data[$instance['field_name']] = $queried_entities[$id]->{$instance['field_name']};
+            $data[$instance['field_name']] = $queried_entities[$id]->{$instance['field_name']}->getValue(TRUE);
           }
           $cid = "field:{$this->entityType}:$id";
           cache('field')->set($cid, $data);

Still, I think we should add some test coverage on a text item to make sure that process value setting and getting works as expected, as do updates on other properties.

yched’s picture

Side note: just stumbled on this change notice, will need to be deleted after this goes in.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
18.26 KB
6.02 KB

Ok. Had to get a bit creative with the tests but this gives us important test coverage for when we're going to change how that works when we change it in other issues like the BC removal or entity-level caching.

yched’s picture

Tests look good, cool ! Only formatting / nitpick remarks:

  1. +++ b/core/modules/text/lib/Drupal/text/Tests/TextWithSummaryItemTest.php
    @@ -0,0 +1,181 @@
    + * Contains \Drupal\text\Tests\TextSummaryItemTest..
    

    Two dots

  2. +++ b/core/modules/text/lib/Drupal/text/Tests/TextWithSummaryItemTest.php
    @@ -0,0 +1,181 @@
    + * Tests the entity API for the taxonomy term reference field type.
    

    Nope :-)

  3. +++ b/core/modules/text/lib/Drupal/text/Tests/TextWithSummaryItemTest.php
    @@ -0,0 +1,181 @@
    +  /**
    +   * Modules to enable.
    +   *
    +   * @var array
    +   */
    +  public static $modules = array('filter');
    

    {@inheritdoc} ?

  4. +++ b/core/modules/text/lib/Drupal/text/Tests/TextWithSummaryItemTest.php
    @@ -0,0 +1,181 @@
    +        /*'filter_autop' => array(
    +          'status' => 1,
    +        ),*/
    

    Commented out ?

  5. +++ b/core/modules/text/lib/Drupal/text/Tests/TextWithSummaryItemTest.php
    @@ -0,0 +1,181 @@
    +  protected function createField($entity_type) {
    

    Ubernitpick: move the helper at the end, not between two test classes that both use it ?

  6. +++ b/core/modules/text/lib/Drupal/text/Tests/TextWithSummaryItemTest.php
    @@ -0,0 +1,181 @@
    +     // Use an entity type that has caching enabled.
    +     $entity_type = 'entity_test_rev';
    

    Indentation is off

Berdir’s picture

Thanks for the review.

- Removed second . ;)
- Yeah, using the pattern that we start using for php unit tests, where we actually reference the class we're testing instead of some random description
- Modules to enable is the current pattern for that, I don't want to break with that here...
- Yeah, I actually want a format that does nothing, didn't update it properly though
- Moved the helper method down
- Fixed indentation.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Yay. RTBC if green.

Berdir’s picture

Oh, I think we can add that @todo now :)

Berdir’s picture

Status: Reviewed & tested by the community » Needs review

Setting back to needs review.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Hah, true.

catch’s picture

Title: Remove text_sanitize() since we have TextProcessed as an EntityNG computed property » Change notice: Remove text_sanitize() since we have TextProcessed as an EntityNG computed property
Category: bug » task
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed/pushed to 8.x, thanks!

Needs a change notice.

Berdir’s picture

Status: Active » Needs review
Berdir’s picture

Also added a reference to https://drupal.org/node/1975504.

yched’s picture

Title: Change notice: Remove text_sanitize() since we have TextProcessed as an EntityNG computed property » Remove text_sanitize() since we have TextProcessed as an EntityNG computed property
Status: Needs review » Fixed
Issue tags: -Needs change record, -sprint

Looks good.

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