Throw exception if invalid method is called instead of silently retur…

[schmengler]

…ning wrong values

Description

In Magento 2.2, a bunch of inherited methods have been deprecated in \Magento\Indexer\Model\Indexer\Collection. Unfortunately they also are overridden to return empty values, which silently breaks integrations which rely on them.

This leads to unnessecarily hard to find bugs and is worse than actually failing. Now they throw BadMethodCall exceptions so that any developer who uses the methods immediately knows why they don't work.

Manual testing scenarios

1. Run the following code, e.g. in an integration test:

        $indexerCollection = $this->objectManager->create(IndexerCollection::class);
        $indexerIds = $indexerCollection->getColumnValues('indexer_id');

2. Previously, $indexerIds was an empty array. After the change, we get an exception.

Contribution checklist

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds on Travis CI are green)

[fooman]

Good catch - would you mind adjusting the unit tests and I'll get this merged.

[schmengler]

Hadn't expected unit tests for this :) Updated!

[sidolov]

Hi all, changes looks good, but according to our backward compatibility policy provided changes are backward incompatible (all client code doesn't expect exceptions after method call). What do you think about it?

[schmengler]

Magento broke backwards compatibility when it started to return empty values. I'd rather have this to be explicit.

[orlangur]

Changes do not look good to me. They go against how deprecation currently occurs and throwing exception is no different from deprecated method removal.

Magento broke backwards compatibility when it started to return empty values

This is not true, empty value is a valid return value for these methods. Backward compatibility is about preserving contracts, not about preserving every single detail of internal implementation.

As to me the best approach for deprecation is the one used by Symfony via trigger_error: https://github.com/symfony/symfony/pull/13060/files#diff-3553121a6c869ab64964d4ee82c9ea32R303 However, it should be discussed on Architecture Council and approved for usage all over the project and cannot be introduced out of the blue in some random pull request.

[schmengler]

You're all looking at backwards compatibility from just the side of "code is valid and executable". But there is more to a contract of a method than its signature and more to backwards compatibility than what static code analysis can tell.

These methods used to return useful data, now they are useless. If I depend on them, my code breaks. And even worse, it breaks without any notice (this is exactly what happened for me, I am not making things up).

However, it should be discussed on Architecture Council and approved for usage all over the project and cannot be introduced out of the blue in some random pull request.

Then please move forward @antonkril

[orlangur]

These methods used to return useful data, now they are useless. If I depend on them, my code breaks. And even worse, it breaks without any notice (this is exactly what happened for me, I am not making things up).

Doing upgrade you need to check whether your code uses some deprecated methods/classes. Ideally at least positive cases should be covered with automated tests.

I'm not saying current deprecation policy is good or bad, I mentioned the one which seems more effective to me. Just that such policy should be used consistently. It is not a good idea if everyone come up with arbitrary fixes "I want to fail fast" done differently. And, again, throwing exception is just like having fatal error due to method removal, such update in patch release would be a bit surprising.

[fooman]

Just to recap 2.2.0 introduced new methods to the Indexer Collection class. The version number of the module was bumped to 100.2.0. New overridden public methods were introduced with a @Deprecation notice:

/**
 * {@inheritdoc} Prevents handle collection items as DataObject class instances.
 * @deprecated 100.2.0 Should not be used in the current implementation.
 * @SuppressWarnings(PHPMD.UnusedFormalParameter)
 */
public function getColumnValues($colName)
{
    return [];
}

from what effectively was before:

/**
 * Retrieve field values from all items
 *
 * @param   string $colName
 * @return  array
 */
public function getColumnValues($colName)
{
    $this->load();
    $col = [];
    foreach ($this->getItems() as $item) {
        $col[] = $item->getData($colName);
    }
    return $col;
}

There is no @api annotation on the class.

If we go by the official definition of what is part of the public api and what isn't @schmengler you are unfortunately out of luck.

However I do agree with your proposed change to make things throw an exception rather than silently changing a method's behaviour (or alternatively bump the major version number with a readme note).

We didn't care about BC when we introduced the change in 100.2.0 so I don't see any reason why we should start to care now when making this more explicit in 100.3.0.

Of course if we could get a global solution that warns in developer mode when a @deprecated method is used that be even better (problem here is of course that Magento itself does this in places...).

[schmengler]

Doing upgrade you need to check whether your code uses some deprecated methods/classes. Ideally at least positive cases should be covered with automated tests.

True. But my argument still stands, what has been done here is not how deprecation is supposed to work.

Even the Magento BC guide states:

Public and protected method removal
Mark the method with the @deprecated tag instead of removing it.

Continue returning the same results from the method if possible, so the old functionality is preserved.

It looks like these methods were meant to be removed, as they are not part of the API and don't work anymore with the current implementation, but it wasn't possible to physically remove them because they were inherited from the base collection.

A decision has been made to implement the removal by turning them into stubs. This was an unfortunate decision for the reasons I stated before and the alternative is to throw an exception.

And, again, throwing exception is just like having fatal error due to method removal

And that exactly seemed to be the intention: method removal

[orlangur]

@schmengler could you please provide a quote from BC guide which states that throwing exception in deprecated method is a good idea?

Continue returning the same results from the method if possible, so the old functionality is preserved.

Stubs = same results, when it is not possible to preserve functionality. Throwing exception is a new, BC breaking, method behavior (usually annotated in PHPDoc). Current implementation is not a BC break, ideally it should be mentioned in release notes also and of course with upgrade your code needs to be checked whether it uses some deprecated methods.

[schmengler]

could you please provide a quote from BC guide which states that throwing exception in deprecated method is a good idea?

No, because it isn't. These methods are essentially removed, not deprecated, that's my point. The major module version already increased IIRC, or it should be. But since we cannot really remove the methods due to inheritance (😠), they stay there, and the @deprecated annotation is a nice way to warn developers who want to use them in advance (PHPStorm for example shows usage of deprecated methods striked-through).

Current implementation is not a BC break

That's where we disagree

[sidolov]

@schmengler , @fooman , @orlangur , guys, I think we need the decision for proposed changes

[schmengler]

I did all I could do. I guess the decision has to be made by the architect counsil as it was mentioned before. @antonkril ?

[buskamuza]

@schmengler , discussed on an arch meeting. We're going to add an item to Tech Guidelines about avoiding such silent behavior change. So in the future, we agree that it would be better to throw an exception in this case. But it's too late to modify it now, as some extensions could already adopt to this new behavior. So here it will stay as is.

[sidolov]

Hi @schmengler , according to the @buskamuza 's comment, I can assume that I can close this PR due to breaking changes.
A corresponded section should be added to the Tech Guideline to avoid such situations.
Thank you for your contribution!