Empty table cells never hidden if twig debug is true

I was having a hard time trying to figure out why the option to hide empty columns wasn't working and found this issue. Here's a quick and dirty patch that fixes this problem for me.

Add a test to check if the field has been excluded.

Here is an updated patch which contains the test against debug mode status.

I'm not quite sure if the regex is totally bullet-proof to exclude HTML comments. Maybe somebody could check this?

Oh, maybe we could implement a dedicated test for that case ? Something like \Drupal\Tests\views\Functional\Plugin\StyleTableTest::testEmptyColumn but with the twig debug enabled.

It's not only views tables problem. I use fields in views. I set "hide field if empty", but it doesn't hide if twig debug is on

Yes, I can confirm this is a bug in views-view-fields as well in support of #13

This also applies if the title for the display is not empty on Drupal 8.7.7.

I added the twig_debug check and stripping and trimming of the field if it's enabled to the isValueEmpty check in FieldPluginBase.php.
This should fix the "hide field if empty" problems for fields in views when twig_debug is enabled. (#13 & #14)

I remade the patch because it failed to apply.

This to me is not a bug, it's not empty => hence it isn't hidden.

But yeah I've run into this too, but still....

Adding a workaround for twig debug changing our definition of empty sounds like a feature (and not one I'm in favour off, turn off twig debug if you want to see what it looks like on production, we don't need more regexes and complexity in the views rendering pipeline, we have plenty of that already). Just my 2ct obviously.

Unfortunately #18 strip also the comment like this "<!--form-item-entity_browser_select--paragraphs_library_item:1-->" and in the result checkboxes not visible:
Without patch: https://take.ms/iDQOU
With patch: https://take.ms/i9g8N

I found this issue with the "Paragraph Library" module.

$value = preg_replace('/<!--.*?-->/s', '', $value);

This seems to break Media Library pretty badly. The checkboxes disappear on each item.

The regex captures and delete the placeholder <!-- -->, e.g <!--form-item-media_library_select_form--0-->. See \Drupal\media_library\Plugin\views\field\MediaLibrarySelectForm::getValue

I just made a small commit that solves for me the issue mentioned in #28.

I also made a merge request.

You can download the patch as a diff directly from my commit. I'm also attaching a patch file here.

Tried to fix CCF for #31.

32 incorrectly introduces a space in the regex.

Noticed a problem with the patch. The following error is raised, likely due to PHP8 changes.

Deprecated function: preg_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated in Drupal\views\Plugin\views\field\FieldPluginBase->isValueEmpty() (line 1226 of core/modules/views/src/Plugin/views/field/FieldPluginBase.php).

We should ensure we don't enter the if (\Drupal::service('twig')->isDebug()) { condition if value is NULL.

Adding a new patch addressing points from #34 and #35, and also fixing some phpcs errors. please review.

1. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -1236,6 +1240,15 @@ public function isValueEmpty($value, $empty_zero, $no_skip_empty = TRUE) {
   +    if ($twig_debug_on == TRUE && !is_null($twig_debug_on)) {
   

   the previous way of doing it, from 32 and before, was fine. no need to assign and check TRUE

   this is also checking the wrong value for whether it is NULL. you should be checking $value. Not the result of isDebug()

   $value !== NULL will be fine for the null check.

2. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -1236,6 +1240,15 @@ public function isValueEmpty($value, $empty_zero, $no_skip_empty = TRUE) {
   +      $value = preg_replace('/<!--.*?-->/s', '', (string) $value);
   

   this correctly removed the leading space

@TanujJain-TJ there are many changes in the new patch compared to the previous without explanation, in addition to an overly large interdiff? Have you brought in irrelevant changes?

Tests continue to fail.

NW

@dpi sorry for that null check i interpreted it wrong at first updating the patch to correct all the changes required, also these out of scope changes are just PHPCS fixes i got while running this below given command, did it so the patch doesn't fail Custom commands check. adding new interdiff with #32.
phpcs --standard="Drupal,DrupalPractice" --extensions="php,module,inc,install,test,profile,theme,css,info,txt,md,yml"

Please address the CS changes relevant to the lines being modified only.

updated according to #40, please review.

@Tanuj ... for tests to pass i think you also need to check the twig service exists

So, maybe you could update patch (or create new MR) with something like:

    // If twig_debug is enabled we strip out the
    // html comments before running the empty check.
    if ($value !== NULL) {
      if (\Drupal::hasService('twig') && \Drupal::service('twig')->isDebug()) {
        $value = preg_replace('/<!--.*?-->/s', '', (string) $value);
        $value = trim($value);
      }
    }