Error due to method return annotation

[sbutnariu]

Since upgrading my legacy project to php 7.4 I get an error regarding the return type of offsetUnset method from Zend_Db_Table_Row_Abstract (Zend/Db/Table/Row/Abstract.php)

The error reads as such: "message":"Error #16384 Method \"ArrayAccess::offsetUnset()\" might add \"void\" as a native return type declaration in the future. Do the same in implementation \"Zend_Db_Table_Row_Abstract\" now to avoid errors or add an explicit @return annotation to suppress this message."

I see that the method has #[\ReturnTypeWillChange], but this is not supported until php8.1.

Th simple fix for this is to do exactly what the error message suggests, adding a return annotation.

[develart-projects]

@sbutnariu could you please add couple of lines with code, so I can reproduce the problem? Thanks.

[develart-projects]

I tried to figure out when void return type has been added, but did not find anything in the PHP 7.x changelogs. But found out, that offsetSet has already void return type, so I'm considering it safe to add as well.

[develart-projects]

..but that's not possible, because

/**
      * Proxy to __unset
      * Required by the ArrayAccess implementation
      *
      * @param string $offset
      */
     #[\ReturnTypeWillChange]
     public function offsetUnset($offset)
     {
         return $this->__unset($offset);
     }

..is just wrapper to

/**
     * Unset row field value
     *
     * @param  string $columnName The column key.
     * @return Zend_Db_Table_Row_Abstract
     * @throws Zend_Db_Table_Row_Exception
     */
    public function __unset($columnName)
    {
        $columnName = $this->_transformColumn($columnName);
        if (!array_key_exists($columnName, $this->_data)) {
            require_once 'Zend/Db/Table/Row/Exception.php';
            throw new Zend_Db_Table_Row_Exception("Specified column \"$columnName\" is not in the row");
        }
        if ($this->isConnected() && in_array($columnName, $this->_table->info('primary'))) {
            require_once 'Zend/Db/Table/Row/Exception.php';
            throw new Zend_Db_Table_Row_Exception("Specified column \"$columnName\" is a primary key and should not be unset");
        }
        unset($this->_data[$columnName]);
        return $this;
    }

..and that's returning itself as fluent interface.

Therefore I'm considering this change as impossible and recommending to use PHP 8, if needed.

[develart-projects]

Closing as Wontfix, because potential fix is a breaking change.

[boenrobot]

Who in their right mind ~would write~ has ever written

$row->__unset($columnA)->offsetUnset($columnB);

?

This is the only way in which a fluent interface on those methods can work. The intended use

unset($row->{$columnA}, $row[$columnB]);

would not break with such a change, and neither would non fluent uses like

$row->__unset($columnA);
$row->offsetUnset($columnB);

[develart-projects]

@boenrobot if anyone uses it this way, it will be broken:

$row->offsetUnset($x)
         ->offsetUnset($y);

Look to me as pretty valid code.

[boenrobot]

The question is how often do we think someone actually used this form over

$row->offsetUnset($x);
$row->offsetUnset($y);

// or
unset($row[$x]);
unset($row[$y]);

unset($row[$x], $row[$y]);

unset($row->{$x});
unset($row->{$y});

unset($row->{$x}, $row->{$y});

Considering that offsetUnset() in ArrayAccess at least was never meant to return a value... It's just that older PHP versions had no way to explicitly denote this in the interface.

Or I suppose the other equally important consideration... If someone does use this fluent form, how much of a hassle is for them to refactor? How often does one do this in an application? Doing the change to non-fluent use would be a change compatible with ZF 1.12, i.e. one can do that change before moving to zf1-future.

In most scenarios I can think of where you'd need to unset multiple columns from a row, you'd want a loop over the columns, in which case you're not affected. And if you only have a few enumerated ones... you'd hope there's a small number of parts of the application that do this, even if said part(s) are very hot paths.

I suppose if static analyzers like Rector supported detecting and fixing this, the process of such a refactor would be mostly painless. Speaking of which, I saw that that they supported it at one point, but I can't find it in the current version. I've asked here rectorphp/rector#8154 .

[develart-projects]

OK, I'm reopening this for discussion. Imo this is breaking change, but I myself do not using this ORM feature, so I haven't any strong opinion on this one.

[develart-projects]

• as we have discussed this already in some other Issue, we have to create some BreakingChanges.md for any such operation.

[boenrobot]

There is a "Known issues and solutions" section in the readme (btw, not sure if that described issue is a real issue anymore... the link makes it seem as if a solution was found...).

But yeah, perhaps a dedicated file that is just linked to from the readme would be better.

In this case, I think text like the following should suffice in such a file

Zend_Db_Table_Row_Abstract::__unset() and Zend_Db_Table_Row_Abstract::unsetOffset() returned $this in zendframework 1.12 and zf1-future before 1.25. They have been changed to void, as per later PHP version's explicit return type annotation. Before migrating to zf1-future 1.25 and later, code that calls those methods directly instead of using unset() should be refactored to not use the return value or use unset(). e.g.

$row->offsetUnset($x)->offsetUnset($y);

should be changed to

$row->offsetUnset($x);
$row->offsetUnset($y);

or

unset($row[$x], $row[$y]);

The #[\ReturnTypeWillChange] attribute and lack of explicit return type hint in both methods can be kept to reduce the impact on custom row implementations.

And while we're at it, maybe add some text about the recent session interface change. That is another one of those that one could technically write cross compatible code for, but may possibly break.

[develart-projects]

This problem is growing bigger over time and this example is just tiny part of the problem. Session interface is another.

For now, we are keeping compatibility mode across v7 and v8 PHP as much as possible. However, in PHP 7 isn't any way how to suppress return types. And forcing big running projects to additional costs by rewriting all the code parts is not the best option.

Probably is time to start discussion (in another thread):

• if we are going to keep v7 support forever
• if not, how the EOL will be set
• if yes, how to approach PHP strict-types streak

[IMSoP]

I just want to point out that the "error" quoted in the top of this issue is very explicitly not intended to be a fatal error, as is clear from its wording: the change "might [happen] in the future", and you should "[do] the same [...] now to avoid errors [in that future version]". If it is preventing you from running your application, that is due to some configuration somewhere else, not in PHP or zf1-future.

I also note that the quoted text does not appear to be the wording produced by PHP itself; as far as I know, PHP does not assign numeric codes to any of its messages. If this is from some code quality checking tool, the above applies even more strongly: it is up to you how you configure that code quality tool. It is generally not possible to maintain a "legacy project" and simultaneously keep up to date with evolving best practices.

[IMSoP]

Further to the above, the "tentative return types" for internal methods were added in PHP 8.1, not 7.4. The #[\ReturnTypeWillChange] attribute was added at the same time, so is always an option to suppress the built-in message.