ampersand-magento2-upgrade-patch-helper
ampersand-magento2-upgrade-patch-helper copied to clipboard
AmpersandHQ
Feature: new error type "Queue consumer added"
In Magento updates often new queue consumers are added. In common server setups there is no automatic run for all consumers, so each needs to be configured separately. So its vital to detect new introduced consumers. This PR introduces this Feature.
@convenient I gave my best in understanding the code and contributing, but if you see a way to improve the PR, feel free to do so :)
Checklist
- [x] Pull request has a meaningful description of its purpose
- [x] All commits are accompanied by meaningful commit messages
- [x] Tests have been ran / updated
@convenient I also wanted to implement the error type "Queue consumers removed" and tested the implementation by removing a queue_consumer.xml in the target project vendor dir and recreating vendor.patch. Test was negative: the patchFiles array does not contain removed file somehow. So the implementation for this error type only worked for cases, when a singel queue consumer was deleted from the file but not the whole file was removed. I did not want to have a partial solution so i did not include it into the PR. If you have an idea how to make this check work correctly, feel free to do so / inform me. Regards Alex
Just back after a period of annual leave. This looks neat, will review once I've cleared by backlog a bit.
Also just in case you're curious, I have managed this a bit differently using static analysis in our projects.
We define all the consumers we want to run in app/etc/config.php so they are committed into the repo.
I can then have the following script run as part of a CI job which will look for a list of all the jobs it can find in the vendor directory. It's not very pretty, but lets us keep on track of any new consumers added at any time be it a module install or whatever
<?php
set_error_handler(function ($severity, $message, $file, $line) {
throw new \ErrorException($message, $severity, $severity, $file, $line);
});
chdir(__DIR__ .'/../../');
$configDotPhp = require './app/etc/config.php';
exec('for i in `find ./vendor -type f -name "*queue_consumer.xml"`; do grep -l "<consumer" $i; done', $output);
$consumerFiles = array_filter($output, function ($line) {
if (stripos($line, 'test') !== false) {
return false;
}
if (stripos($line, '/dev/') !== false) {
return false;
}
return true;
});
sort($consumerFiles);
$disabledConsumers = [
'async.operations.all', // uses amqp so we disable it
];
$activeConsumers = $configDotPhp['cron_consumers_runner']['consumers'];
$existingConsumers = [];
foreach ($consumerFiles as $consumerFile) {
$file = \simplexml_load_file($consumerFile);
foreach ($file->children() as $tagname => $value) {
$existingConsumers[] = (string)$value->attributes()['name'];
}
}
if (empty($existingConsumers)) {
echo 'Not a single consumer file was found or scanned? Seems like a possible error';
exit(1);
}
// Remove any we have hard disabled
$intendedActiveConsumers = array_diff($existingConsumers, $disabledConsumers);
// Compare to make sure we have hard coded all consumers we need to enable or disable
sort($activeConsumers);
sort($intendedActiveConsumers);
$difference = array_diff($intendedActiveConsumers, $activeConsumers);
if ($difference) {
echo 'You are missing the following from your app/etc/config.php consumers list, or from the $disabledConsumers list above' . PHP_EOL;
echo 'or if it is not necessary or uses ampq from the $disabledConsumers list above' . PHP_EOL;
print_r($difference);
exit(1);
}
echo "DONE" . PHP_EOL;
exit(0);
Hello @convenient, I've just tested the PR again locally and found out, that completely new queue_consumer.xml files which were added are not detected... So please wait a bit with merging this, i'll check if something can be done.
@a-dite does it work if you do diff -ur -N vendor_orig/ vendor/ > vendor.patch ?
I think it won't be flagging any issues as this is mainly for detecting overwrites/overrides, and it assumed that if something new being added didn't previously exist it's not worth looking at.
The -N should produce a + diff entry line for each line in a new file which may work
@convenient Yeah, just checked diff -urN vendor_orig/ vendor/ > vendor.patch which is pretty the same :) Yes, then it works and in my example (2.3.7 --> 2.4.3 Update) it shows then 7 added queue consumers instead of 3, because 4 are added in new files... Also, with the -N option removed files are shown with a - diff entry line. WIth that, i can also add new error type "Queue consumers removed".
Wouldn't the -N option be meaningful also for the core UpgradeHelper functionality? I mean you write
and it assumed that if something new being added didn't previously exist it's not worth looking at.
But with the current -ur option the deleted files are also ignored. But i guess if smthing is deleted, its worth looking at :)
What do you think?
@a-dite I thought that might be it 😬
It's kind of how this tool is opinionated and focussing in on only things that change when you upgrade. So if something new is added, you can't have overridden it so it goes on by.
I've just created a branch to let the CI pipeline show what kind of effects this has against the test suites https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/compare/master...see-how-new-files-are-parsed?quick_pull=1
@convenient I think the introduced feature here only makes sense with the -N option for diff command. So I've added another commit with 2nd error type "Queue consumers removed" because it's working also correctly with -N option. I also update documentation (README.md) in this commit and updated the diff -ur ... to diff -ur -N instruction there. Feel free to adapt the Tests :) Or give me an instruction, I can also do it
Hey @a-dite if you cherry pick in c1087ece216e194bd70e87c402c7c9ffb3388057 there are only two test files needing updated to get it ✅
https://app.travis-ci.com/github/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/jobs/581582666
There were 2 failures:
1) FunctionalTests::testMagentoTwoThree
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
| Override (phtml/js/html) | vendor/magento/module-customer/view/frontend/email/password_reset_confirmation.html | app/design/frontend/Ampersand/theme/Magento_Customer/email/password_reset_confirmation.html |\n
| Override (phtml/js/html) | vendor/magento/module-customer/view/frontend/templates/account/dashboard/info.phtml | app/design/frontend/Ampersand/theme/Magento_Customer/templates/account/dashboard/info.phtml |\n
| Override (phtml/js/html) | vendor/magento/module-sales/view/frontend/layout/sales_order_print.xml | app/design/frontend/Ampersand/theme/Magento_Sales/layout/sales_order_print.xml |\n
+| Override (phtml/js/html) | vendor/magento/module-ui/view/base/web/templates/grid/masonry.html | app/design/frontend/Ampersand/theme/Magento_Ui/web/templates/grid/masonry.html |\n
| Override (phtml/js/html) | vendor/magento/module-vault/view/frontend/web/js/view/payment/method-renderer/vault.js | app/design/frontend/Ampersand/theme/Magento_Vault/web/js/view/payment/method-renderer/vault.js |\n
| Plugin | vendor/magento/framework/Stdlib/Cookie/PhpCookieManager.php | Ampersand\Test\Plugin\PhpCookieManager::beforeSetPublicCookie |\n
| Plugin | vendor/magento/module-sales/Block/Adminhtml/Order/View/History.php | Ampersand\Test\Block\Plugin\OrderViewHistoryPlugin::afterCanSendCommentEmail |\n
/src/dev/phpunit/functional/FunctionalTests.php:84
phpvfscomposer:///src/vendor/phpunit/phpunit/phpunit:97
2) FunctionalTests::testMagentoTwoThreeShowCustomModules
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
| Override (phtml/js/html) | vendor/magento/module-customer/view/frontend/email/password_reset_confirmation.html | app/design/frontend/Ampersand/theme/Magento_Customer/email/password_reset_confirmation.html |\n
| Override (phtml/js/html) | vendor/magento/module-customer/view/frontend/templates/account/dashboard/info.phtml | app/design/frontend/Ampersand/theme/Magento_Customer/templates/account/dashboard/info.phtml |\n
| Override (phtml/js/html) | vendor/magento/module-sales/view/frontend/layout/sales_order_print.xml | app/design/frontend/Ampersand/theme/Magento_Sales/layout/sales_order_print.xml |\n
+| Override (phtml/js/html) | vendor/magento/module-ui/view/base/web/templates/grid/masonry.html | app/design/frontend/Ampersand/theme/Magento_Ui/web/templates/grid/masonry.html |\n
| Override (phtml/js/html) | vendor/magento/module-vault/view/frontend/web/js/view/payment/method-renderer/vault.js | app/design/frontend/Ampersand/theme/Magento_Vault/web/js/view/payment/method-renderer/vault.js |\n
| Plugin | vendor/magento/framework/Stdlib/Cookie/PhpCookieManager.php | Ampersand\Test\Plugin\PhpCookieManager::beforeSetPublicCookie |\n
| Plugin | vendor/magento/module-checkout/CustomerData/Cart.php | Amazon\Core\Plugin\CartSection::afterGetSectionData |\n
/src/dev/phpunit/functional/FunctionalTests.php:102
phpvfscomposer:///src/vendor/phpunit/phpunit/phpunit:97
Thanks @a-dite i will review more thoroughly tomorrow :)
@hostep I see you've 🎉 'd this, any thoughts ? :)
And @peterjaap the recommended diff command may be changing, not sure if that affects anything on your GUI tooling or any CI you've hooked up.
It sounds like a really good addition to me! Thanks @a-dite!
Maybe similar logic can be used for discovering changes in other type of xml files (like adminhtml/system.xml to see if configuration settings were added/removed). Just an idea, not sure if really helpful 🙂
@convenient as long as the output still resembles the previous output, we're fine. The GUI doesn't do anything with the command per se, only with its output.
@peterjaap output is the same now only contains new row types like
| Preference | vendor/magento/module-weee/Model/Total/Quote/Weee.php | Ampersand\Test\Model\Total\Quote\Weee |
+ | Queue consumer added | vendor/magento/module-inventory-indexer/etc/queue_consumer.xml | inventory.indexer.sourceItem |
Maybe similar logic can be used for discovering changes in other type of xml files (like adminhtml/system.xml to see if configuration settings were added/removed). Just an idea, not sure if really helpful
neat idea @hostep this kind of thing is definitely now possible, but I wonder how much noise we'd get for how much value? I fear the payment methods and third party modules magento bundles / unbundles would cause this to be loud? 🤔
Great stuff @a-dite , this is all merged/tagged/tests passing. it's been a while since we've had an actual new feature on this tool :) Really appreciated, if you have any other neat ideas please hit me up.
neat idea @hostep this kind of thing is definitely now possible, but I wonder how much noise we'd get for how much value? I fear the payment methods and third party modules magento bundles / unbundles would cause this to be loud? 🤔
True, but the behavior could maybe be hidden behind a flag? --also-show-config-changes (I'm bad at naming things, so this is probably not the best option)
@convenient @a-dite I just ran the new version on an actual upgrade project and got this;
| Queue consumer added | vendor/magento/module-catalog/etc/queue_consumer.xml | product_action_attribute.update |
| Queue consumer added | vendor/magento/module-catalog/etc/queue_consumer.xml | product_action_attribute.website.update |
| Queue consumer removed | vendor/magento/module-catalog/etc/queue_consumer.xml | product_action_attribute.update |
| Queue consumer removed | vendor/magento/module-catalog/etc/queue_consumer.xml | product_action_attribute.website.update |
| Queue consumer added | vendor/magento/module-import-export/etc/queue_consumer.xml | exportProcessor |
| Queue consumer removed | vendor/magento/module-import-export/etc/queue_consumer.xml | exportProcessor |
| Queue consumer added | vendor/magento/module-inventory-catalog/etc/queue_consumer.xml | inventory.source.items.cleanup |
| Queue consumer added | vendor/magento/module-inventory-catalog/etc/queue_consumer.xml | inventory.mass.update |
| Queue consumer removed | vendor/magento/module-inventory-catalog/etc/queue_consumer.xml | inventory.source.items.cleanup |
| Queue consumer removed | vendor/magento/module-inventory-catalog/etc/queue_consumer.xml | inventory.mass.update |
| Queue consumer added | vendor/magento/module-inventory-indexer/etc/queue_consumer.xml | inventory.reservations.updateSalabilityStatus |
| Queue consumer added | vendor/magento/module-inventory-indexer/etc/queue_consumer.xml | inventory.indexer.sourceItem |
| Queue consumer added | vendor/magento/module-inventory-indexer/etc/queue_consumer.xml | inventory.indexer.stock |
| Queue consumer removed | vendor/magento/module-inventory-indexer/etc/queue_consumer.xml | inventory.reservations.updateSalabilityStatus |
| Queue consumer removed | vendor/magento/module-inventory-indexer/etc/queue_consumer.xml | inventory.indexer.sourceItem |
| Queue consumer removed | vendor/magento/module-inventory-indexer/etc/queue_consumer.xml | inventory.indexer.stock |
| Queue consumer added | vendor/magento/module-inventory-sales/etc/queue_consumer.xml | inventory.reservations.update |
| Queue consumer added | vendor/magento/module-inventory-sales/etc/queue_consumer.xml | inventory.reservations.cleanup |
| Queue consumer removed | vendor/magento/module-inventory-sales/etc/queue_consumer.xml | inventory.reservations.update |
| Queue consumer removed | vendor/magento/module-inventory-sales/etc/queue_consumer.xml | inventory.reservations.cleanup |
| Queue consumer added | vendor/magento/module-media-storage/etc/queue_consumer.xml | media.storage.catalog.image.resize |
| Queue consumer removed | vendor/magento/module-media-storage/etc/queue_consumer.xml | media.storage.catalog.image.resize |
| Queue consumer added | vendor/magento/module-sales-rule/etc/queue_consumer.xml | codegeneratorProcessor |
| Queue consumer added | vendor/magento/module-sales-rule/etc/queue_consumer.xml | sales.rule.update.coupon.usage |
| Queue consumer added | vendor/magento/module-sales-rule/etc/queue_consumer.xml | sales.rule.quote.trigger.recollect |
| Queue consumer removed | vendor/magento/module-sales-rule/etc/queue_consumer.xml | codegeneratorProcessor |
| Queue consumer removed | vendor/magento/module-sales-rule/etc/queue_consumer.xml | sales.rule.update.coupon.usage |
| Queue consumer added | vendor/magento/module-webapi-async/etc/queue_consumer.xml | async.operations.all |
| Queue consumer removed | vendor/magento/module-webapi-async/etc/queue_consumer.xml | async.operations.all |
It seems it now lists all queue consumers as both removed and added?
@peterjaap What version moving from and to?
and chance you can share the relevant part of the diff for vendor/magento/module-sales-rule/etc/queue_consumer.xml ?
@convenient from 2.4.3-p1 to 2.4.5
diff --color -ur vendor_orig/magento/module-sales-rule/etc/queue_consumer.xml vendor/magento/module-sales-rule/etc/queue_consumer.xml
--- vendor_orig/magento/module-sales-rule/etc/queue_consumer.xml 2021-07-13 23:08:00.000000000 +0200
+++ vendor/magento/module-sales-rule/etc/queue_consumer.xml 2022-07-21 10:45:10.000000000 +0200
@@ -6,6 +6,7 @@
*/
-->
<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:framework-message-queue:etc/consumer.xsd">
- <consumer name="codegeneratorProcessor" queue="codegenerator" connection="db" maxMessages="5000" consumerInstance="Magento\Framework\MessageQueue\Consumer" handler="Magento\SalesRule\Model\Coupon\Consumer::process" />
- <consumer name="sales.rule.update.coupon.usage" queue="sales.rule.update.coupon.usage" connection="db" maxMessages="5000" consumerInstance="Magento\Framework\MessageQueue\Consumer" handler="Magento\SalesRule\Model\CouponUsageConsumer::process" />
+ <consumer name="codegeneratorProcessor" queue="codegenerator" handler="Magento\SalesRule\Model\Coupon\Consumer::process" />
+ <consumer name="sales.rule.update.coupon.usage" queue="sales.rule.update.coupon.usage" handler="Magento\SalesRule\Model\CouponUsageConsumer::process" />
+ <consumer name="sales.rule.quote.trigger.recollect" queue="sales.rule.quote.trigger.recollect" handler="Magento\SalesRule\Model\Queue\Consumer\RuleQuoteRecollectTotals::process" />
</config>
That explains it. In this commit, they removed some attributes.
Maybe we could add some logic that reads the XML line and checks the name. If the name is the same, but the other attributes changed, maybe add a line "Queue consumer changed" or something?
Thanks @peterjaap I should be able to get a test case in for this and work it out.
Hey @convenient and @peterjaap, yeah, now seeing this huge Magento commit where all queue consumer files were refactored, I admit the logic to detect added / changed / removed queue consumers should be a bit more sophisticated than in my implementation. @convenient If you need some help, let me know. I have also another raw implementation on my machine, which I later decided not to submit because its actually fully implemented in the "AnalyseCommand" class and is a bit workaround of that how this UpgradeHelper tool works. I attach the file here so you can check if its maybe nevertheless a safer way to find added, modified, removed consumers... AnalyseCommand.php.txt
Fixed the above https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/releases/tag/v1.10.1