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().

Files: 
CommentFileSizeAuthor
#32 text-processed-caching-2026339-32.patch18.02 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,493 pass(es).
[ View ]
#32 text-processed-caching-2026339-32-interdiff.txt926 bytesBerdir
#30 text-processed-caching-2026339-30.patch18.2 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#30 text-processed-caching-2026339-30-interdiff.txt3.98 KBBerdir
#28 text-processed-caching-2026339-28-test-only.patch6.02 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,490 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#28 text-processed-caching-2026339-28.patch18.26 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,404 pass(es).
[ View ]
#23 text-processed-caching-2026339-23.patch12.25 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,424 pass(es).
[ View ]
#23 text-processed-caching-2026339-23-interdiff.txt2.47 KBBerdir
#17 text-processed-caching-2026339-17.patch13.97 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,612 pass(es).
[ View ]
#17 text-processed-caching-2026339-17-interdiff.txt3.52 KBBerdir
#12 text-processed-caching-2026339-12.patch12.22 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,613 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#12 text-processed-caching-2026339-12-interdiff.txt5.82 KBBerdir
#5 processed_check_markup_xhprof.png194.65 KBBerdir
#5 text-processed-caching-2026339-5.patch10.6 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,475 pass(es), 17 fail(s), and 0 exception(s).
[ View ]

Comments

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.

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.

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

Retitling

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.

Status:Active» Needs review
Issue tags:+API change
StatusFileSize
new10.6 KB
FAILED: [[SimpleTest]]: [MySQL] 58,475 pass(es), 17 fail(s), and 0 exception(s).
[ View ]
new194.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.

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.

  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 :-(

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.

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

Status:Needs work» Needs review
Issue tags:+Needs tests
StatusFileSize
new5.82 KB
new12.22 KB
FAILED: [[SimpleTest]]: [MySQL] 58,613 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

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.

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)

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.

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 {
(...)

Status:Needs work» Needs review
StatusFileSize
new3.52 KB
new13.97 KB
PASSED: [[SimpleTest]]: [MySQL] 58,612 pass(es).
[ View ]

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.

+++ 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...

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.

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 ?

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

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.

StatusFileSize
new2.47 KB
new12.25 KB
PASSED: [[SimpleTest]]: [MySQL] 58,424 pass(es).
[ View ]

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.

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.

Status:Reviewed & tested by the community» Needs work

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

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.

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

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new18.26 KB
PASSED: [[SimpleTest]]: [MySQL] 58,404 pass(es).
[ View ]
new6.02 KB
FAILED: [[SimpleTest]]: [MySQL] 58,490 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

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.

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

StatusFileSize
new3.98 KB
new18.2 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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.

Status:Needs review» Reviewed & tested by the community

Yay. RTBC if green.

StatusFileSize
new926 bytes
new18.02 KB
PASSED: [[SimpleTest]]: [MySQL] 58,493 pass(es).
[ View ]

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

Status:Reviewed & tested by the community» Needs review

Setting back to needs review.

Status:Needs review» Reviewed & tested by the community

Hah, true.

Title:Remove text_sanitize() since we have TextProcessed as an EntityNG computed propertyChange 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.

Status:Active» Needs review

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

Title:Change notice: Remove text_sanitize() since we have TextProcessed as an EntityNG computed propertyRemove 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.