Criteria problem with empty value

[GregMage]

In the development of one of my modules, I want to test whether a field in my table is empty (in a search). To do this, I've written: $criteria_field->add(new Criteria('fielddata_value1', '')); The Criteria.php class (render function) blocks this entry with the following code: if ('' === ($value = trim((string)$this->value))) { return ''; } For me, this behavior is problematic. I suggest replacing the code with: if ('' === ($value = trim((string)$this->value))) { $value = ' '; } With this code, the criterion is added without errors.

What do you think?

[mambax7]

By assigning a space character to empty values, you may introduce unexpected behavior in other parts of the code that rely on the Criteria class. It could lead to inconsistencies or incorrect results in certain scenarios.

An alternative approach would be to modify the add() method in your module to handle empty values explicitly. You can check if the value is empty before adding the criterion. Here's an example:

if (!empty($fielddata_value1)) {
    $criteria_field->add(new Criteria('fielddata_value1', $fielddata_value1));
}

This way, the criterion will only be added if the $fielddata_value1 variable is not empty.

To test the behavior, you can create a test class. Here's an example test class using PHPUnit:

<?php

use PHPUnit\Framework\TestCase;

class CriteriaTest extends TestCase
{
    public function testAddEmptyValue()
    {
        $criteria = new Criteria('fielddata_value1', '');
        $this->assertEmpty($criteria->render());
    }

    public function testAddNonEmptyValue()
    {
        $criteria = new Criteria('fielddata_value1', 'test');
        $this->assertNotEmpty($criteria->render());
    }

    public function testAddEmptyValueWithModifiedCode()
    {
        // Assuming you have modified the Criteria class code as suggested
        $criteria = new Criteria('fielddata_value1', '');
        $this->assertNotEmpty($criteria->render());
        $this->assertEquals(' ', $criteria->getValue());
    }
}

In this test class, we have three test methods:

1. testAddEmptyValue(): Tests adding an empty value to the Criteria class using the original code. It asserts that the rendered output is empty.

2. testAddNonEmptyValue(): Tests adding a non-empty value to the Criteria class. It asserts that the rendered output is not empty.

3. testAddEmptyValueWithModifiedCode(): Tests adding an empty value to the Criteria class using the modified code as you suggested. It asserts that the rendered output is not empty and that the value is a single space character.

You can run these tests using PHPUnit to verify the behavior of the Criteria class with empty and non-empty values.

Another approach would be to introduce a way to handle empty strings explicitly in your criteria. This can be achieved by extending the Criteria class or adding a method that specifically handles empty string queries. Here is how you could implement this in a more controlled and transparent manner:

1. Extend the Criteria class to add functionality for handling empty strings:

   class CriteriaAllowEmpty extends Criteria
   {
   public function render(): string
   {
       // Allow empty strings as a valid query value
       if ('' === trim((string)$this->value)) {
           return sprintf(" %s %s '' ", $this->prefix, $this->column);
       }
   
       return parent::render();
   }
   }

   This solution avoids changing the global behavior of the Criteria class across your application and confines the new behavior to cases where it is explicitly needed. Additionally, it ensures that you're not affecting other modules or future updates to the core Criteria class that might rely on the existing behavior of ignoring empty strings.

This way we can ensure that we follow SOLID principles where applicable, particularly the Open/Closed Principle by extending the class instead of modifying it.

[GregMage]

Hi, there,

The code I proposed is not good, you need to provide something else. For my application I extended the base class. This solution works very well. The problem is that the Criteria class must accept a '' value. This is a useful feature. So we need to come up with a solution to this problem that won't compromise code compatibility. Perhaps we could add a parameter to the class to allow this option when we want it?

By default, nothing happens, but if necessary, we can activate the option. I think this is a good solution.

Translated with DeepL.com (free version)

[mambax7]

We could introduce an optional parameter to the Criteria class constructor to control this behavior. This way, the default behavior remains unchanged, but you have the flexibility to allow empty string values when needed.

Here's a suggested modification to the Criteria class:

class Criteria
{
    // ...

    protected $allowEmptyValue = false;

    public function __construct($column, $value, $operator = '=', $allowEmptyValue = false)
    {
        $this->column = $column;
        $this->value = $value;
        $this->operator = $operator;
        $this->allowEmptyValue = $allowEmptyValue;
    }

    // ...

    public function render()
    {
        if ('' === ($value = trim((string)$this->value)) && !$this->allowEmptyValue) {
            return '';
        }

        // ...
    }

    // ...
}

In this modification:

1. We introduced a new protected property $allowEmptyValue with a default value of false. This property determines whether empty string values are allowed or not.

2. We added an optional parameter $allowEmptyValue to the constructor, which defaults to false. This parameter allows you to specify whether empty string values should be allowed when creating a new Criteria instance.

3. In the render() method, we modified the condition to check both the trimmed value and the $allowEmptyValue property. If the value is an empty string and $allowEmptyValue is false, an empty string is returned. Otherwise, the rendering proceeds as usual.

With this modification, the default behavior of the Criteria class remains the same, but you have the option to allow empty string values by passing true as the fourth argument when creating a new instance. For example:

$criteria_field->add(new Criteria('fielddata_value1', '', '=', true));

This way, you can explicitly allow empty string values for specific criteria when needed, without affecting the behavior of existing code that relies on the default behavior.

To test this modification, you can update the test class as follows:

public function testAddEmptyValueWithAllowEmptyValueOption()
{
    $criteria = new Criteria('fielddata_value1', '', '=', true);
    $this->assertNotEmpty($criteria->render());
    $this->assertEquals('', $criteria->getValue());
}

This test method verifies that when the $allowEmptyValue option is set to true, an empty string value is accepted and rendered correctly.

Would it work for you?

@geekwright What do you think?

[GregMage]

I'm sorry I didn't reply. It's true that adding a parameter is the best choice for compatibility reasons.