I'm aware of #617596: Non-empty Summary and empty Body both become empty when saved.. This actually a follow-on. I was doing some profiling of htmlpurifier and noticed something strange with really short entries in a node's 'value'. So short that there is no 'summary'. It still passed the text value of the body to filter.module. This is unnecessary, since the text is an empty string.
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | 1821178-17.patch | 581 bytes | poker10 |
| #15 | drupal-performance-tune-1821178-14.patch | 1.42 KB | heddn |
| #11 | drupal-text_field_load-1821178-11.patch | 1.37 KB | heddn |
| #8 | drupal-text_field_load-1821178-8.patch | 1.44 KB | heddn |
| #8 | xhprof-callgraph-1821178.png | 49.45 KB | heddn |
Comments
Comment #1
heddnComment #2
heddnOops, added patch to the wrong issue. Ignore #1.
Comment #3
penyaskitoGo testbot, go!
Comment #5
heddndrupal-d8-text_field_load.patch queued for re-testing.
Comment #6
penyaskitoThe patch at the summary seems to do the work and tests are green.
Comment #7
catchThis ternary is getting very long:
Could we split it into an if?
Would also be good to do an xhprof run just to show there's no need to format this. Seems like this one is backportable to 7.x as well.
Comment #8
heddnI tried running xhprof on D8, but I couldn't easily port htmlpurifier since the caching mechanism changes so dramatically in D8. Here's a callgraph from xhprof (D7). Its only called from _field_invoke_multiple when I view the content of a node that fires the htmlpurifier filter imediately after clearing cache. And a reformatted patch with if/else clauses.
Comment #9
effulgentsia commentedComment #11
heddnRerolled
Comment #12
Stalski commented#11: drupal-text_field_load-1821178-11.patch queued for re-testing and if green, bumping this issue
Comment #14
heddnThis doesn't need to be applied to D8 in its current state. The logic to handle empty summaries was put in place with a2c2367. So, this simply needs a backport.
Comment #15
heddnPatch submitted by the Drush iq-submit command.
Comment #16
jenlamptonthis may be a dupe of #1533276: Micro optimization in text_field_load
Comment #17
poker10 commentedThere are 8 usages of the
_text_sanitize()function in D7 core (plus some usages in tests): https://api.drupal.org/api/drupal/modules%21field%21modules%21text%21text.module/function/calls/_text_sanitize/7.xWouldn't it be better to apply the non-empty check directy into the
_text_sanitize()function, which would cover all other usages? Otherwise this patch is optimizing only the_text_sanitize()call in thetext_field_load()function (not sure if this partial effort is worth the effort). But please correct me if I am wrong or if I missed something.Attaching a simple patch what I had in mind.
Comment #18
poker10 commentedThere are two points to consider:
1. The last patch will not catch
NULLvalues (which can happen on PHP 8+). These NULLs are handled by string conversion incheck_markup, see:This is not optimal, but changing to patch to catch all possible empty input values will be a bigger change. I think the current change is a safer approach.
2. We can consider moving the empty check even deeper - to the
check_markup.Adding a tag so that another D7 maintainer can have a look.
Comment #20
mcdruid commentedYeah it's tempting to just check for an
empty()value in the column but as you say probably safer to keep to this very straightforward change especially as we're not adding any tests here.Thanks everyone!