Closed Tjitse-E closed 4 years ago
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!
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.
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?
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!
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.
Merged #3, which fixes this issue, thanks again!
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 🙂
@hostep i've prepared a PR that will fix this issue in the core: https://github.com/magento/magento2/pull/26538
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.
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.