Integration tests failing after module installation

[Tjitse-E]

After installing this module via composer, our integration testing suite broke. This is because of the three console commands that this module provides. I'll create a PR it fix this in a minute.

[hostep]

Hmm, that's odd.

It feels more like a dependency was not declared correctly which makes this module load earlier than it should load. But I'm wondering which one (Magento_Store is in the dependency list already, which should create that store_website table).

Adding proxies probably will fix this, but I'm not sure yet if that is the most correct solution here.

Have you seen this sort of problem happen before with other modules, and was the best way to fix that by introducing proxies?

I'll see if I can reproduce this somehow in the next few days, it suffices to run the built-in integration tests of Magento with this module installed?

Thanks a lot for the issue and for the PR btw!

[Tjitse-E]

I'll see if I can reproduce this somehow in the next few days, it suffices to run the built-in integration tests of Magento with this module installed?

Yes, it will then try to setup Magento with an emtpy DB and then the error will be trown.

Have you seen this sort of problem happen before with other modules, and was the best way to fix that by introducing proxies?

Yes, in most cases adding a dependency in module.xml is enough. But i'm 90% certain that with console commands you also need to add proxies via DI.xml for certain classes. I always solve this with a proxies, it seems like a clean solution.

[hostep]

Alrighty, I was able to reproduce the problem, here is the stack trace:

Exception trace:
  at lib/internal/Magento/Framework/DB/Statement/Pdo/Mysql.php:91
 PDOStatement->execute() at lib/internal/Magento/Framework/DB/Statement/Pdo/Mysql.php:91
 Magento\Framework\DB\Statement\Pdo\Mysql->Magento\Framework\DB\Statement\Pdo\{closure}() at lib/internal/Magento/Framework/DB/Statement/Pdo/Mysql.php:107
 Magento\Framework\DB\Statement\Pdo\Mysql->tryExecute() at lib/internal/Magento/Framework/DB/Statement/Pdo/Mysql.php:92
 Magento\Framework\DB\Statement\Pdo\Mysql->_execute() at vendor/magento/zendframework1/library/Zend/Db/Statement.php:303
 Zend_Db_Statement->execute() at vendor/magento/zendframework1/library/Zend/Db/Adapter/Abstract.php:480
 Zend_Db_Adapter_Abstract->query() at vendor/magento/zendframework1/library/Zend/Db/Adapter/Pdo/Abstract.php:238
 Zend_Db_Adapter_Pdo_Abstract->query() at lib/internal/Magento/Framework/DB/Adapter/Pdo/Mysql.php:546
 Magento\Framework\DB\Adapter\Pdo\Mysql->_query() at lib/internal/Magento/Framework/DB/Adapter/Pdo/Mysql.php:621
 Magento\Framework\DB\Adapter\Pdo\Mysql->query() at vendor/magento/zendframework1/library/Zend/Db/Adapter/Abstract.php:737
 Zend_Db_Adapter_Abstract->fetchAll() at app/code/Magento/Store/App/Config/Source/RuntimeConfigSource.php:87
 Magento\Store\App\Config\Source\RuntimeConfigSource->getEntities() at app/code/Magento/Store/App/Config/Source/RuntimeConfigSource.php:57
 Magento\Store\App\Config\Source\RuntimeConfigSource->get() at lib/internal/Magento/Framework/App/Config/ConfigSourceAggregated.php:40
 Magento\Framework\App\Config\ConfigSourceAggregated->get() at dev/tests/integration/tmp/sandbox-0-2f9c871904439a2fdd87b5c78577c3031333aee15d5cc2c6454e3d80df3cfafc/generated/code/Magento/Framework/App/Config/ConfigSourceAggregated/Proxy.php:95
 Magento\Framework\App\Config\ConfigSourceAggregated\Proxy->get() at app/code/Magento/Store/App/Config/Type/Scopes.php:63
 Magento\Store\App\Config\Type\Scopes->get() at lib/internal/Magento/Framework/App/Config.php:132
 Magento\Framework\App\Config->get() at app/code/Magento/Store/Model/WebsiteRepository.php:198
 Magento\Store\Model\WebsiteRepository->initDefaultWebsite() at app/code/Magento/Store/Model/WebsiteRepository.php:156
 Magento\Store\Model\WebsiteRepository->getDefault() at app/code/Magento/Store/Model/StoreResolver/Website.php:49
 Magento\Store\Model\StoreResolver\Website->getAllowedStoreIds() at app/code/Magento/Store/Model/StoresData.php:65
 Magento\Store\Model\StoresData->getStoresData() at app/code/Magento/Store/Model/StoreResolver.php:138
 Magento\Store\Model\StoreResolver->getStoresData() at app/code/Magento/Store/Model/StoreResolver.php:97
 Magento\Store\Model\StoreResolver->getCurrentStoreId() at app/code/Magento/Store/Model/StoreManager.php:160
 Magento\Store\Model\StoreManager->getStore() at dev/tests/integration/tmp/sandbox-0-2f9c871904439a2fdd87b5c78577c3031333aee15d5cc2c6454e3d80df3cfafc/generated/code/Magento/Store/Model/StoreManagerInterface/Proxy.php:119
 Magento\Store\Model\StoreManagerInterface\Proxy->getStore() at app/code/Magento/Store/Model/Resolver/Store.php:30
 Magento\Store\Model\Resolver\Store->getScope() at lib/internal/Magento/Framework/App/Config/ScopeCodeResolver.php:49
 Magento\Framework\App\Config\ScopeCodeResolver->resolve() at lib/internal/Magento/Framework/App/Config.php:69
 Magento\Framework\App\Config->getValue() at lib/internal/Magento/Framework/Stdlib/DateTime/Timezone.php:111
 Magento\Framework\Stdlib\DateTime\Timezone->getConfigTimezone() at lib/internal/Magento/Framework/Stdlib/DateTime/DateTime.php:35
 Magento\Framework\Stdlib\DateTime\DateTime->__construct() at lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php:116
 Magento\Framework\ObjectManager\Factory\AbstractFactory->createObject() at lib/internal/Magento/Framework/ObjectManager/Factory/Dynamic/Developer.php:66
 Magento\Framework\ObjectManager\Factory\Dynamic\Developer->create() at lib/internal/Magento/Framework/ObjectManager/ObjectManager.php:70
 Magento\Framework\ObjectManager\ObjectManager->get() at lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php:160
 Magento\Framework\ObjectManager\Factory\AbstractFactory->resolveArgument() at lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php:246
 Magento\Framework\ObjectManager\Factory\AbstractFactory->resolveArgumentsInRuntime() at lib/internal/Magento/Framework/ObjectManager/Factory/Dynamic/Developer.php:34
 Magento\Framework\ObjectManager\Factory\Dynamic\Developer->_resolveArguments() at lib/internal/Magento/Framework/ObjectManager/Factory/Dynamic/Developer.php:59
 Magento\Framework\ObjectManager\Factory\Dynamic\Developer->create() at lib/internal/Magento/Framework/ObjectManager/ObjectManager.php:70
 Magento\Framework\ObjectManager\ObjectManager->get() at lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php:160
 Magento\Framework\ObjectManager\Factory\AbstractFactory->resolveArgument() at lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php:246
 Magento\Framework\ObjectManager\Factory\AbstractFactory->resolveArgumentsInRuntime() at lib/internal/Magento/Framework/ObjectManager/Factory/Dynamic/Developer.php:34
 Magento\Framework\ObjectManager\Factory\Dynamic\Developer->_resolveArguments() at lib/internal/Magento/Framework/ObjectManager/Factory/Dynamic/Developer.php:59
 Magento\Framework\ObjectManager\Factory\Dynamic\Developer->create() at lib/internal/Magento/Framework/ObjectManager/ObjectManager.php:70
 Magento\Framework\ObjectManager\ObjectManager->get() at lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php:160
 Magento\Framework\ObjectManager\Factory\AbstractFactory->resolveArgument() at lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php:246
 Magento\Framework\ObjectManager\Factory\AbstractFactory->resolveArgumentsInRuntime() at lib/internal/Magento/Framework/ObjectManager/Factory/Dynamic/Developer.php:34
 Magento\Framework\ObjectManager\Factory\Dynamic\Developer->_resolveArguments() at lib/internal/Magento/Framework/ObjectManager/Factory/Dynamic/Developer.php:59
 Magento\Framework\ObjectManager\Factory\Dynamic\Developer->create() at lib/internal/Magento/Framework/ObjectManager/ObjectManager.php:70
 Magento\Framework\ObjectManager\ObjectManager->get() at lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php:196
 Magento\Framework\ObjectManager\Factory\AbstractFactory->parseArray() at lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php:172
 Magento\Framework\ObjectManager\Factory\AbstractFactory->resolveArgument() at lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php:246
 Magento\Framework\ObjectManager\Factory\AbstractFactory->resolveArgumentsInRuntime() at lib/internal/Magento/Framework/ObjectManager/Factory/Dynamic/Developer.php:34
 Magento\Framework\ObjectManager\Factory\Dynamic\Developer->_resolveArguments() at lib/internal/Magento/Framework/ObjectManager/Factory/Dynamic/Developer.php:59
 Magento\Framework\ObjectManager\Factory\Dynamic\Developer->create() at lib/internal/Magento/Framework/ObjectManager/ObjectManager.php:56
 Magento\Framework\ObjectManager\ObjectManager->create() at setup/src/Magento/Setup/Model/ObjectManagerProvider.php:78
 Magento\Setup\Model\ObjectManagerProvider->createCliCommands() at setup/src/Magento/Setup/Model/ObjectManagerProvider.php:64
 Magento\Setup\Model\ObjectManagerProvider->get() at setup/src/Magento/Setup/Model/Installer.php:821
 Magento\Setup\Model\Installer->installSchema() at n/a:n/a
 call_user_func_array() at setup/src/Magento/Setup/Model/Installer.php:367
 Magento\Setup\Model\Installer->install() at setup/src/Magento/Setup/Console/Command/InstallCommand.php:221
 Magento\Setup\Console\Command\InstallCommand->execute() at vendor/symfony/console/Command/Command.php:255
 Symfony\Component\Console\Command\Command->run() at vendor/symfony/console/Application.php:934
 Symfony\Component\Console\Application->doRunCommand() at vendor/symfony/console/Application.php:273
 Symfony\Component\Console\Application->doRun() at lib/internal/Magento/Framework/Console/Cli.php:105
 Magento\Framework\Console\Cli->doRun() at vendor/symfony/console/Application.php:149
 Symfony\Component\Console\Application->run() at bin/magento:23

Looking through it, it seems like instantiating a Magento\Framework\Stdlib\DateTime\DateTime object needs to be able to load configuration values out of the database. But if certain tables aren't installed yet in the database, this fails.
This feels like a bug in Magento, Magento's framework shouldn't try to talk to the database for instantiating DateTime objects in my opinion.

Anyway, as a fix we should probably proxy the DateTime instantiating in Baldwin\UrlDataIntegrityChecker\Storage\Meta.

I got the integration tests running by replacing the following in that Meta class:

use Magento\Framework\Stdlib\DateTime\DateTime;

with:

use Magento\Framework\Stdlib\DateTime\DateTime\Proxy as DateTime;

I'm not sure yet what I prefer, doing the proxying in di.xml or in the php class itself.
Using xml is probably cleaner. But might lead to oversights when the argument in the php class gets changed. So I think I currently prefer changing the argument type in the php class instead of in the xml file.

Thoughts? Does this makes any sense?

[hostep]

But then again, Magento2's Coding Standard doesn't seem to like this:

vendor/bin/phpcs --standard=Magento2 --ignore=./vendor/ .

FILE: Storage/Meta.php
----------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------------
 24 | ERROR | Proxies and interceptors MUST never be explicitly requested in constructors.
----------

So we should then still go for the proxying defined in the di.xml file.

Could you update your PR to proxy the DateTime argument in the Baldwin\UrlDataIntegrityChecker\Storage\Meta class in the di.xml file? I think that might be the cleanest solution for now.
Thanks!

[Tjitse-E]

Yes, good solution, the DateTime class is indeed the cause of this.

This is the constructor of that class:

     * @param TimezoneInterface $localeDate
     */
    public function __construct(TimezoneInterface $localeDate)
    {
        $this->_localeDate = $localeDate;
        $this->_offset = $this->calculateOffset($this->_localeDate->getConfigTimezone());
    }

It's failing because of the getConfigTimezone() that's in the constructor, which does a call to the DB via scopeConfigInterface. So this seems like a Magento bug indeed.

I'll update the PR in a minute.

[hostep]

Merged #3, which fixes this issue, thanks again!

[hostep]

Just FYI: since you guys already make use of the module and there seems to be some interest in the module by others, I decided to publish the package to packagist.org and tagged a version v1.0.0, so you may want to update the projects where you make use of this module 🙂

[Tjitse-E]

@hostep i've prepared a PR that will fix this issue in the core: magento/magento2#26538

[hostep]

Oeh, awesome, good job, I'll keep my eye on it to see if it gets merged!
If it gets approved, I won't be removing the proxy here, because we still need to support older Magento versions.