Load services.php before configuration to allow container customizations

[olsavmic]

Resolves qossmic/deptrac#1419

[gennadigennadigennadi]

How does this help with overriding deptrac internal stuff?
And does this have an performance impact on cache projects?

FYI: I'll try to do some testing next week.

[olsavmic]

How does this help with overriding deptrac internal stuff?

It allows you to define services section in the configuration file which can override the defaults from services.php. It used to work like this in 1.x (https://github.com/qossmic/deptrac-src/blob/1.0.1/src/Supportive/DependencyInjection/ServiceContainerBuilder.php)

And does this have an performance impact on cache projects?

I believe that the impact should be negligible, as the cache is still resolved as the very last step (once all services from both the services file and configuration are resolved).

Better to properly measure than to just hypothesize, of course :D Thank you!

[olsavmic]

@gennadigennadigennadi Do you have some specific set of benchmarks in mind? I can help you test that.

I don't see any reason why the runtime performance should be affected by my changes. The flow is following:

1. Load default services definition into the container builder
2. Load configuration (which includes the cache file definition)
   • this stage allows default service overrides
3. Load the cacheFile (based on the configuration param)
4. Build the DIC container (only at this point are the services, params etc. actually resolved) from the builder

I just ran the patched version on our codebase, and the cache works just fine :) And the service overrides as well.

---

I have added one more commit, to make the "service override experience" unified. I'll test the results again.

[olsavmic]

The total runtime for v1.0.2, 2.0.0 without patch and both patches here on our codebase (15 000+ files) is very consistent without any signs of degradation.

[gennadigennadigennadi]

I just went through the changes and hadn't had enough time to properly think about the changes.

Right now I'm not sure what gets cached at all? I don't think the service definitions are.

Also, even if this changes get merged, there is no guarantee it will stay like this. This is nothing but a hack, until Deptrac officially exposes its internals.

And I don't think we will expose the Symfony container officially.

[patrickkusebauch]

I think this is fine. It would be great if it included a test for the order of operation so that the next refactor of the loader does not break this again.

Furthermore, I think it would be good to expose more of Deptrac Interfaces in the contract. The individual Deptrac modules have been stable for quite a while now in terms of module boundaries. If @olsavmic and @janedbal could share the interfaces Shipmonk would be interested in overwriting, I could make some effort into putting them into the contract namespace.

[olsavmic]

I have reverted the last commit to prevent the creation of a cache file when --no-cache was used, keeping the original behavior. The pipeline should pass now.

[olsavmic]

@patrickkusebauch We have two main use-cases, already discussed in:

• Unable to use custom EventSubscriberInterface in depfile since 2.0 as those are prefixed in the phar deptrac#1417 (for custom baseline format and custom violation exclusion)
• Ability to add custom PhpParser\NodeVisitor to alter AST attributes deptrac#1402 (described in Ability to add custom PhpParser\NodeVisitor to alter AST attributes deptrac#1402 (comment), this one is really touching the internals and is the main reason we need this MR - it's definitely not a standard, but the tool is very well extensible with help of DIC)

[patrickkusebauch]

I talked to Jan over lunch lasts week and we discussed having a Parser interface for you to use your custom parsing logic for creating the AST references and a Baseline mapper interface for you to have a custom baseline mapping implementation.

[olsavmic]

@gennadigennadigennadi Can you please approve the workflow? Thank you!

[olsavmic]

The pipeline is green, can I please get a review & merge?

Even if we work on the extension points, this change helps to test customizations more quickly 🤔

Thank you!

[gennadigennadigennadi]

I think this is fine. It would be great if it included a test for the order of operation so that the next refactor of the loader does not break this again.

I would really like to see a test for it, too. Otherwise it looks good.