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

Decorator extension used with Search extension #215

Closed
solcik opened this issue Aug 6, 2019 · 11 comments
Closed

Decorator extension used with Search extension #215

solcik opened this issue Aug 6, 2019 · 11 comments

Comments

@solcik
Copy link

solcik commented Aug 6, 2019

Version: 3.0.0

            "name": "nette/di",
            "version": "v3.0.0",
            "source": {
                "type": "git",
                "url": "https://github.com/nette/di.git",
                "reference": "19d83539245aaacb59470828919182411061841f"
            },

Bug Description

When I use Decorator together with Search extension it does not decorate services. It is in the DI, but does not have tag.

If I insert service definition directly without the use of Search extension, then it does work as expected.

Steps To Reproduce

decorator:
	Doctrine\Common\EventSubscriber:
		tags: [nettrine.subscriber]
search:
	-
		in: %appDir%/Subscriber
<?php

declare(strict_types=1);

namespace Acme\Subscriber;

use Doctrine\Common\EventSubscriber;

final class LifecycleSubscriber implements EventSubscriber
{}

Expected Behavior

Acme\Subscriber\LifecycleSubscriber class should be in DI decorated with tag nettrine.subscriber.

@dg
Copy link
Member

dg commented Aug 6, 2019

It should be fixed by #197, can you check it out?

@solcik
Copy link
Author

solcik commented Aug 6, 2019

Ouch, sorry did not see that. Yeah it fixes the problem. Thank you!

Now it is correctly in the DI set with tag.

Unfortunately, it created a new problem xD

Since nettrine/dbal's extension is now before the Decorator. It's beforeCompile does not know about the tag, because it is not yet assigned, but that discussion I will move to nettrine/dbal issue.

https://github.com/nettrine/dbal/blob/master/src/DI/DbalExtension.php#L155

@dg
Copy link
Member

dg commented Aug 7, 2019

I think this is a more complex problem. I will not release the version with #197 yet.

@dg dg reopened this Aug 7, 2019
@peldax
Copy link
Contributor

peldax commented Aug 18, 2019

@dg Another way could be adding Search and Decorator (in this order) to the $first.

Parameter -> Extensions -> Search -> Decorator -> ... other ... -> Services

That way services from Search would be decorated correctly and any other extensions relating on tags from Decorator would be satisfied. On the other hand, this would break extensions which only add services and rely on decorator to decorate them - which is a bad idea anyway.

Edit: It might also break extensions adding services manually because Search would register them before...

@mabar
Copy link
Contributor

mabar commented Aug 18, 2019

Currently I assume in:

  • loadConfiguration() are registered services
  • beforeCompile() are services modified, connected to each other (if it was not done in loadConfiguration() and eventually removed or replaced

I think we all have some expectances in which order thinks should happen. Unfortunately it will be never same for all of us. Search and decorator extension do to much, both of them simply cannot work in every single case -> there would be always some issues.

If we register search extension in $first -> services from searched directory cannot be registered manually -> it would need to be removed in beforeCompile() and with it comes expectance service was not explicitly used by an extension (e.g. console command service name added to console lazy loader) -> no way

If search extension is in $last -> with expectance services not directly registered by extension are worked with in beforeCompile() it should imho work. But it would need decorator extension to be registered also in $last, after search extension -> decorator would be useless in compile-time while run-time behavior would remain uneffected. At least for extensions which uses tags intensively (kdyby/console) it's a no way

Search or decorator extensions not prioritized? We cannot expect anything -> random errors, biggest no way.

Even prioritization of extensions based on hierarchy of composer dependencies would not work in this specific case.

My proposal is complete removal of the expectations. Whole beforeCompile() phase (or at least what I expect should happen in this phase) could be replaced by event system.

Conceptually it adds listeners which:

  • are added in new method, let's call it setupListeners()
  • listen to things like 'A service of type FooInterface added', Service foo.bar.baz type changed to FooInterface, Service lorem.ipsum removed, Any service set to type FooInterface, Tag abcd added to aby service, ...

Phase 1 - Extensions setup their event listeners
Phase 2 - Extensions register their services (which does not have external dependencies)
Phase 3 - Services from config are loaded
Phase 4 - Events stacked during phase 2 and 3 are triggered -> services from extensions and config are connected
Phase 5 - Compilation
Phase 6 - After compilation modifications

Phases as I wrote comes with expectance events are triggered after all services are registered. But if addition of services would be allowed in events phase (phase 4) then we should ensure only fully initialized, valid services are added to container builder (which would call event dispatcher) so modification events are not triggered instead of creation events and changes from events are not mixed into initialization. This would need instead $builder->addDefinition(): Definition to have $builder->addDefinition(Definition $definition)

Only thing I worry about is that with great power there must also come great responsibility. Events over-usage may cause cyclical dependencies, so we would need a loop detection.

@peldax
Copy link
Contributor

peldax commented Aug 18, 2019

@mabar I was thinking in same ways as you. About the expectations and that every possible solution breaks in some situation, but I believe your proposed approach is overly complex and difficult to maintain.

Simple reordering from my previous comment breaks manual registration of services.

I would suggest to reorder - search and decorator to $first - but change behaviour so that manual registration replaces services from search extension.

@peldax
Copy link
Contributor

peldax commented Aug 18, 2019

Simply adding decorator to $last is OK for my case, because I use decorator for setup, not for tags. At the time of original PR it didn't really occurred to me it would break decorator-tags usage.

@dakorpar
Copy link

dakorpar commented Jan 30, 2020

any news about this?
What about adding setup options directly to search extension?
Then it's not really consistent since decorator couldn't be used with auto registered services and it doesn't even solve issue, but at least it would allow setup on autoregistered services and currently we cannot use this plugin at all with any service layer that require some setup. Also search extension is already doing some part of decorator, for example you can set tags...

edit: more I think I believe solution with allowing setup directly in search extension would actually be only possible solution, otherwise you're allways in issues with priority of what loads when. If stated in docs that search extension supports all stuff like decorator and that decorator cannot be in use with services registered with search extension it sounds like only possible solution ATM that won't break anything and allow full functionality....

@dg
Copy link
Member

dg commented Mar 11, 2020

should be fixed in master

@FVesely
Copy link

FVesely commented Apr 7, 2020

@dg Still not working for this use case.

<?php declare(strict_types = 1);

namespace App\Components;

abstract class Component
{
	protected Translator $translator;

	public function setTranslator(Translator $translator): void
	{
		$this->translator = $translator;
	}
}

final class Foo extends Component
{
}

interface FooFactory
{
	public function create(): Foo;
}
search:
	default:
		in: %appDir%
		classes:
			- App\Components\*Factory
decorator:
	App\Components\Component:
		setup:
			- setTranslator

@dg
Copy link
Member

dg commented May 8, 2020

@FVesely fixed

@dg dg closed this as completed May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants