Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Missing dependencies / setup fails on new installations #2509

Merged
merged 9 commits into from
Mar 7, 2024

Conversation

pmzandbergen
Copy link
Contributor

@pmzandbergen pmzandbergen commented Feb 13, 2024

Missing dependency may cause new installations to fail.
candemiralp
candemiralp previously approved these changes Feb 13, 2024
hossam-adyen
hossam-adyen previously approved these changes Feb 13, 2024
@hossam-adyen hossam-adyen self-requested a review February 13, 2024 14:02
@hossam-adyen hossam-adyen removed their request for review February 13, 2024 14:03
@pmzandbergen pmzandbergen marked this pull request as draft February 13, 2024 14:55
@pmzandbergen pmzandbergen changed the title fix: Missing dependency for Magento_Customer module fix: Missing dependencies / setup fails on new installations Feb 13, 2024
@pmzandbergen
Copy link
Contributor Author

@hossam-adyen these problems were introduced by: #2476

@bramstroker will add the details

@bramstroker
Copy link
Contributor

Narrowed down the issue to be in https://github.com/Adyen/adyen-magento2/blob/develop/Setup/Recurring.php

During execution of the recurring schema the following components will be constructed by DI container:

  • \Adyen\Payment\Setup\Recurring
  • \Adyen\Payment\Helper\PaymentMethodsFactory
  • \Adyen\Payment\Helper\Data
  • \Magento\Tax\Model\Calculation
  • \Magento\Customer\Model\Session
  • \Magento\Customer\Model\ResourceModel\Customer

Construction of Customer resource calls setType and fails with following exception, thus crashing Magento installation.

Invalid entity_type specified: customer

The eav entity types are created by data patch in Magento_Customer module, and data patches are being executed after recurring schema.

The question is why this is in recurring schema in the first place as it has nothing to do with DB schema. It just sets some config settings.

So the solution seems to me to execute this logic in a data patch, which ultimately also has a dependency on the Magento data patch which creates the eav_entity_type records. \Magento\Customer\Setup\Patch\Data\DefaultCustomerGroupsAndAttributes

@hossam-adyen
Copy link
Contributor

Hi @pmzandbergen, thanks for your valuable contribution and dedicate time for opening this draft and providing this fix, please let us know when it's ready for review.

If you also don't mine, please update the unit test file to avoid the failure of the PR checks

@pmzandbergen
Copy link
Contributor Author

Hi @pmzandbergen, thanks for your valuable contribution and dedicate time for opening this draft and providing this fix, please let us know when it's ready for review.

If you also don't mine, please update the unit test file to avoid the failure of the PR checks

Have a look at: #2509 (comment)

@bramstroker
Copy link
Contributor

I have changed current Recurring schema script, to RecurringData, which solves our problems.

Workflow for CI/CD testing pipeline in our projects is as follows:

  • composer install using existing composer.lock file, containing both magento installation and 3rd party modules (also the adyen module).
  • bin/magento setup:install

This fails without the fixes provided in this PR.

It would be nice if similar procedure also can be verified in you QA pipeline, as it's not the first time our CI/CD pipelines break after (minor) upgrade of Adyen module.

@bramstroker
Copy link
Contributor

Can this be merged please so it can be part of upcoming release?
This currently blocks updates for the module for our clients.
Don't like to put any temporary composer patches to work around the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Indicates a bug fix
Projects
None yet
4 participants