When "Hide rewriting if empty" is selected and "Count the number 0 as empty" is not selected, the number 0 is not displayed. Unchecking "Hide rewriting if empty" causes the rewritten field to be displayed.

This patch moves the rewriting after the empty check.

CommentFileSizeAuthor
#21 views-1433596-21.patch15.75 KBtim.plunkett
#18 views-1433596-18.patch15.75 KBtim.plunkett
#17 views-1433596-17-test-only.patch12.42 KBtim.plunkett
#17 views-1433596-17.patch15.75 KBtim.plunkett
#13 views-1433596-13-test-only.patch12.68 KBtim.plunkett
#13 views-1433596-13.patch16.01 KBtim.plunkett
#9 views-1433596-9.patch3.33 KBtim.plunkett
#8 views_rewrite_empty_check-1433596-8.patch982 bytesimclean
#6 views_rewrite_empty_check-1433596-6.patch941 bytesimclean
#2 views_rewrite_empty_check-1433596-2.patch693 bytesimclean
views-rewrite-empty-check.patch1.21 KBimclean
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

imclean’s picture

imclean CreditAttribution: imclean commented
Status: Needs work » Needs review

Wrong status.

Possibly related: #1428256: No results behavior "Hide rewriting if empty " is not working

imclean’s picture

imclean CreditAttribution: imclean commented
FileSize
views_rewrite_empty_check-1433596-2.patch693 bytes

I've gone about this the wrong way. The problem seems to be the empty check is performed on the rewritten value ($value) on the second pass rather than the original value.

This means the following, after being rewritten, would evaluate to true:

if ((($this->options['hide_empty'] && empty($value))
        || ($this->options['hide_alter_empty'] && empty($this->original_value)))
      && (($value !== 0 && $value !== '0')
        || $this->options['empty_zero'])) {
      return '';
    }

Specifically, the following should be false in the case of zero:

&& (($value !== 0 && $value !== '0')

Checking the original value instead gives the expected response:

&& (($this->original_value !== 0 && $this->original_value !== '0')

Possibly also related: #1273486: “Empty Text” does not work when “Hide Rewriting if field is empty" option is selected.

imclean’s picture

imclean CreditAttribution: imclean commented

#2 won't work. $this->original_value isn't set if the field isn't being rewritten. Maybe the first approach was better.

Does the empty check really need to be done on the rewritten value? I guess potentially the original field could be "empty" while the rewritten field may or may not produce output using tokens + html, but "hide if empty" should take care of this.

I think "Hide if empty" should always hide the field, including any rewriting. What would be the purpose of rewriting the field before performing the empty check?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Needs work

Is this still required on the current version? There was some logic applied recently. Additional the patch does definitive not apply anymore.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Is this still required on the current version? There was some logic applied recently. Additional the patch does definitive not apply anymore.

imclean’s picture

imclean CreditAttribution: imclean commented
Status: Needs work » Needs review
FileSize
views_rewrite_empty_check-1433596-6.patch941 bytes

Same problem with the current version. empty() always considers 0 to be empty.

The following returns true when hide_alter_empty is set and the original_value is 0, regardless of empty_zero setting.

$no_rewrite_for_empty = $this->options['hide_alter_empty'] && empty($this->original_value);

This patch adds some extra checks. I'm not sure if it's the most efficient method but it works for me.

imclean’s picture

imclean CreditAttribution: imclean commented
Status: Needs review » Needs work

Gah, same mistake as before. If the value hasn't been rewritten, $this->original_value may not be set to check for 0 so an error is produced.

$value may be the rewritten value so that can't be checked either.

imclean’s picture

imclean CreditAttribution: imclean commented
Status: Needs work » Needs review
FileSize
views_rewrite_empty_check-1433596-8.patch982 bytes

Try again.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
views-1433596-9.patch3.33 KB

The patch doesn't address all situations.

I checked both a non-empty field rewritten to 0, and an empty field rewritten to 10, with all possible combinations of the checkboxes.

This adds a helper function, because the !== 0/'0' pattern appears in 4 distinct places, one of them wrong.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: Unassigned » dawehner 
+++ b/handlers/views_handler_field.incundefined
@@ -1062,6 +1062,27 @@ If you would like to have the characters %5B and %5D please use the html entity
+    if ($check_empty) {
+      $empty = empty($value) && $empty;
+    }

I strongly debated leaving this part out of the function and just calling empty() in the code, but I do believe this is cleaner.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue tags: +Needs tests

It would be kind of cool to extend the already existing tests. This code has to many cases that you can be sure all the time.
Introducing a helper function definitive makes things a bit clearer.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: dawehner » tim.plunkett

I'm working on the tests.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
views-1433596-13.patch16.01 KB
views-1433596-13-test-only.patch12.68 KB

Alright, I went a little overboard on the tests, but they needed to be cleaned up.

Should have about 2 fails on the test only patch, and all passes on the other.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: tim.plunkett » dawehner

Well you'd have to run it locally to see that. Simpletest > Views Handler > Field.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

Triggering the testbot.

Status: Needs review » Needs work

The last submitted patch, views-1433596-13.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
views-1433596-17.patch15.75 KB
views-1433596-17-test-only.patch12.42 KB

Reroll

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
views-1433596-18.patch15.75 KB

In IRC @dereine suggested switching the default value for is_value_empty().

Status: Needs review » Needs work

The last submitted patch, views-1433596-18.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review 

     $view->field['name']->options['alter']['alter_text'] = TRUE;
-    $view->result[0]->{$column_map_reversed['name']} = "";
-    $random_name = $this->randomName();
     $view->field['name']->options['alter']['text'] = $random_name;
+
+    // Test a valid string.
+    $view->result[0]->{$column_map_reversed['name']} = $random_name;
+    $render = $view->field['name']->advanced_render($view->result[0]);
+    $this->assertIdentical($render, $random_name, 'If the rewritten string is not empty, it should not be treated as empty.');
+

Just a small issue: It could be that the rewritten value is used, as both are equal, what about creating another random name for the actual value?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
views-1433596-21.patch15.75 KB

Duh, I switched which function calls used the third parameter, but not what they were passing. Those fails were a good thing :)

Also included the suggestion from #20.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Version: 7.x-3.x-dev » 6.x-3.x-dev
Status: Needs review » Patch (to be ported)

The patch is fine and has a phenomenal test coverage. Let's get it in! Thanks for the work!

Chris Matthews’s picture

Chris Matthews CreditAttribution: Chris Matthews as a volunteer commented
Issue summary: View changes
Status: Patch (to be ported) » Closed (outdated)

The Drupal 6 branch is no longer supported, please check with the D6LTS project if you need further support. For more information as to why this issue was closed, please see issue #3030347: Plan to clean process issue queue