search

deprecated-packages / symplify

[DISCONTINUED] Check split packages in their own repositories :)
MIT License
621 stars 189 forks source link

[DefaultAutowire] Incompatibility with symfony/monolog-bundle 3.0 #32

Closed enumag closed 7 years ago

enumag commented 7 years ago

Steps to reproduce:

symfony new monolog-test
cd monolog-test/
composer require symplify/default-autowire
php bin/console server:start

Now add DefaultAutowireBundle to AppKernel.php.

In the browser you should see this error.

Unable to autowire argument index 1 ($excludedUrls) for the service "monolog.activation_strategy.not_found". If this is an object, give it a type-hint. Otherwise, specify this argument's value explicitly.
TomasVotruba commented 7 years ago

This seems strange to me. So first start a server, then add Bundle to AppKernel?

enumag commented 7 years ago

The order doesn't matter.

TomasVotruba commented 7 years ago

Thanks. Have you tried debugging and find the issue?

Could you check the version of this package installed? I recall this was some issue in the past versions.

enumag commented 7 years ago

It should be the latest - composer outdated didn't mention this package.

I tried to debug it but only briefly. It seems that the extension didn't properly configure the parameters of this service for some reason. But I didn't have time to go further. I don't quite understand why Symfony didn't fail on it when this package wasn't install - need to check the compiled source what arguments were passed.

TomasVotruba commented 7 years ago

Ok, sounds bit complicated for now. I hope you will figure it out.

If you send failing test, I think I can fix it.

enumag commented 7 years ago

Quick update of what I found out. I should be able to add a failing test when I find some time. The problem is probably in DefinitionAnalyzer::haveMissingArgumentsTypehints() - I think it returns true when it shouldn't.

TomasVotruba commented 7 years ago

I see. Feel free to fix it without test, it might be less work with more efficiency.

TomasVotruba commented 7 years ago

What needs to be done here?

enumag commented 7 years ago

Simple, I need to get some time to work on it. :-D

TomasVotruba commented 7 years ago

So not relevant issue in your app? :)

enumag commented 7 years ago

Oh it is relevant, just not a priority at the moment.

TomasVotruba commented 7 years ago

Ok, thanks for update.

enumag commented 7 years ago

@TomasVotruba This was fixed in 1.4.9 but is broken again in 1.4.10. Reopen please.

TomasVotruba commented 7 years ago

I'm sorry about that. I recall I got lost in naming and tests.

This DefinitionAnalyzer requires great refactoring.

TomasVotruba commented 7 years ago

Related to https://github.com/Symplify/Symplify/issues/161 and other feedback I got I wonder, should we exclude 3rd party bundles and limit this just to /app and /src scope?

It would make sense to me and prevent such issues and redundant checking of 3rd party bundles.

enumag commented 7 years ago

Sounds good.

TomasVotruba commented 7 years ago

:+1:

Discussion continue here: https://github.com/Symplify/Symplify/issues/161#issuecomment-299229518

Could you express your opinion on there as well? Now it's just 2 opposite ones.

enumag commented 7 years ago

Maybe tomorrow. It's kind of TL;DR and I have many things to do today.

TomasVotruba commented 7 years ago

I understand that, thank you. Here is tl;dr; summary:

https://github.com/Symplify/Symplify/issues/161#issuecomment-299243468

TomasVotruba commented 7 years ago

Based on discussion in Pehapkari slack, https://pehapkari.slack.com/archives/C2R38JZV3/p1493917175282658, I might consider deprecating it since Symfony 3.3.

I got feedback it's not as useful as it seems.

TomasVotruba commented 7 years ago

Based on community feedback, I've decided to this bundle from Symfony 3.3 on.

See https://github.com/Symplify/Symplify/pull/162