Adding in customisable exclude paths in setup:di:compile

[PivitParkour94]

Description (*)

There has been lots of demand for extending the di compilation exclude paths (either via CLI options or events) so I decided to add in an event setup_di_compile_excluded_patterns that can be observed and added to from within a third-party module.

Fixed Issues (if relevant)

1. [M2.1] - No CLI option to exclude patterns on setup:di:compile and other inflexibilities #5676: [M2.1] - No CLI option to exclude patterns on setup:di:compile and other inflexibilities

Manual testing scenarios (*)

1. Create a module that normally breaks setup:di:compile
2. Add an observer for the 'setup_di_compile_excluded_patterns' event that adds an exclusion path for the file that is breaking the compilation
3. Run bin/magento setup:di:compile and see it compiles as expected despite the bad code in the module file.

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)

Notes

There are a bunch of commits on here, but as you can tell from the diff, those proposed changes that were rejected previously have been reverted. This simply updates the setup:di:compile to give the ability to exclude custom paths via an observer.

[magento-engcom-team]

Hi @PivitParkour94. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento-engcom-team give me test instance - deploy test instance based on PR changes
• @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

[orlangur]

Current implementation excludes tests

magento2/setup/src/Magento/Setup/Console/Command/DiCompileCommand.php

Lines 143 to 154 in 989af63

	$excludedModulePaths = [];
	foreach ($modulePaths as $appCodePath) {
	$excludedModulePaths[] = '#^' . $appCodePath . '/Test#';
	}
	$excludedLibraryPaths = [];
	foreach ($libraryPaths as $libraryPath) {
	$excludedLibraryPaths[] = '#^' . $libraryPath . '/([\\w]+/)?Test#';
	}
	$this->excludedPathsList = [
	'application' => $excludedModulePaths,
	'framework' => $excludedLibraryPaths
	];

and is just fine until there is an evidence something else needs to be excluded from compilation.

[m2-assistant]

Hi @PivitParkour94, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

[jwundrak]

@orlangur We have in some modules a directory "Example", that contains some standalone php files. They are also to check some connectivity. One example: Simple SOAP-Queries. On each setup:di:compile, all of the files are executed.
Ok, this is no "evidence", that this pull request is really needed, but this is my case. Maybe @PivitParkour94 can share us his use case.

Cause we can not use plugins here and a di.xml-argument passing excludePatterns is override by DiCompileCommand:: configureObjectManager and there is no other possibility, to customize the patterns. Adding this event is smart, of course if only <0,1% of developers need this (e.g. there are some questions on magento-stackexchange).

Also there was such an option #986

[m2-assistant]

Hi @PivitParkour94. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

[orlangur]

We have in some modules a directory "Example", that contains some standalone php files. They are also to check some connectivity

@jwundrak why not put them in Test directory? You shouldn't add any junk to your modules in the first place.

[jwundrak]

We have in some modules a directory "Example", that contains some standalone php files. They are also to check some connectivity

@jwundrak why not put them in Test directory? You shouldn't add any junk to your modules in the first place.

Sure, an option. I have noticed, you don't want this option and that is ok.
We use custom patterns now with a plugin. For people, that want to do this also, is here a gist.

Thanks for answer. I close this request again.

[m2-assistant]

Hi @PivitParkour94, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

[orlangur]

Thanks for understanding @jwundrak. Main problem I see is that with such option we can unintentionally affect classes in some other module rather the one we want to exclude from compilation.

[PivitParkour94]

... Maybe @PivitParkour94 can share us his use case.

@jwundrak I've given up trying to help make Magento open source better for developers. It is clear that Magento is focusing on the Commerce market and I wouldn't be surprised if the Open Source dev is frustratingly difficult intentionally, to help incentivise the upgrade to Commerce.

Such is life, I suppose.

[orlangur]

@PivitParkour94 could you please be more specific? This particular decision makes Magento BETTER for developers. You need to do things properly instead of adding some hacks/flags/exclusions here and there.

[PivitParkour94]

This particular decision makes Magento BETTER for developers. You need to do things properly instead of adding some hacks/flags/exclusions here and there.

@orlangur I completely agree, however, we've noticed that almost every 3rd party module developer including modules that are bundled in with the core, don't always follow those practices, which means that we then have to patch OTHER's code to change the "hacks/flags/exclusions here and there" simply because the flexibility isn't there.

I am simply offering a solution to account for the code that isn't up to standard while still allowing a development workflow that doesn't break the entire pipeline.

@PivitParkour94 could you please be more specific?

I've made a number of different Pull Requests, which takes effort and they have been declined and it's just gotten to the point where we maintain a bunch of patches to core that we have to update ourselves with each new version.

It's not as though I'm suggesting a change that forces everyone to change, but rather accommodates those edge cases in which the 0.1% of developers have previously been restricted.

@jwundrak has shown a gist that achieves very similar functionality so clearly this issue effects at least both of our environments.

[orlangur]

@PivitParkour94,

almost every 3rd party module developer including modules that are bundled in with the core, don't always follow those practices

This is something core developers are being working on hard now. Trying to catch as much guidekines violations as possible.

[PivitParkour94]

This is something core developers are being working on hard now. Trying to catch as much guidekines violations as possible.

@orlangur even if that is the case, nothing is going to stop 3rd party developers from trying to force their own development practices into the modules rather than take the time to learn how Magento 2 should be developed and extended.

It's clear that this is not going to be solved and we'll just have to deal with it.

Having said that, in a perfect world everyone would develop according to the standards and things would never break, but this is Software Dev, far from a perfect world.

[jasonhildebrand]

I have a real-world use case. In our Magento instance, we use a 3rd party module called mconnect. It is installed (via composer), and it is used by a custom module (MConnect) in our source tree.

mconnect is installed only in staging and production environments. This is partly due to licensing, and partly because it syncs data with other systems - we don't want to impact these systems from our dev instances. In development, we simply leave MConnect disabled.

However, running bin/magento setup:di:compile fails when it scans the MConnect code, because the classes from mconnect which it requires are not installed. Ideally, we'd have a way to exclude MConnect from being scanned, since we are leaving it disabled and do not use it in dev environments.

[orlangur]

@jasonhildebrand,

In development, we simply leave MConnect disabled.

You clearly don't need it on dev instances. So either strip it during deploy on dev or add it dynamically during deploy on staging/production.

[orlangur]

The rule of thumb is simple: don't keep in source code what you actually don't use. As a side effect you may avoid a lot of troubles.