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

SearchExtension: ignores classes that do not have autowired parameters #316

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

h4kuna
Copy link
Contributor

@h4kuna h4kuna commented Jun 6, 2024

  • new feature
  • BC break? no
  • doc PR: no

Hello,
I use SearchExtension and I have primitive classes in project, but all these classes I must register to exclude section. I think if the class does not have autowired parameters it can be skipped.

Example:

Simple class

namespace Garage\Model;

class Foo
{
	public function __construct(
		public float $precipitation,
	)
	{
	}
}

Actual behavior throw exception:

  • Nette\DI\ServiceCreationException: Service of type Garage\Model\Foo: Parameter $precipitation in Foo::__construct() has no class type or default value, so its value must be specified. in /path/DI/Resolver.php:635

In this moment I must add class to search -> exclude.

New bahevior

If SearchExtension skip this class and I don't need dependency by another class, everything is ok.

If I need dependency by another class, for example Garage\Model\Baz. I get exception:

  • Nette\DI\ServiceCreationException: Service of type Garage\Model\Baz: Service of type Garage\Model\Foo required by $messenger in Baz::__construct() not found. Did you add it to configuration file?

I must manually register the class and I think it is legit. I can't think of a use case where it might be undesirable.

After apply this patch, the result is:
Snímek obrazovky z 2024-06-07 06-32-12

@JanTvrdik
Copy link
Contributor

This would make it hard to reason about why some service is not registered.

@h4kuna
Copy link
Contributor Author

h4kuna commented Jun 6, 2024

@JanTvrdik could you show me an example?

@h4kuna
Copy link
Contributor Author

h4kuna commented Jun 9, 2024

I want only reduce the exclude list, where you don't need to register services.

@h4kuna
Copy link
Contributor Author

h4kuna commented Jun 18, 2024

The similar condition is \ReflectionClass($class)::isInstantiable().

@dg
Copy link
Member

dg commented Jun 18, 2024

This is an absolutely fatal BC break.

@h4kuna
Copy link
Contributor Author

h4kuna commented Jun 19, 2024

Could you more explain the BC break to me, because I don't understand.

@mabar
Copy link
Contributor

mabar commented Jun 19, 2024

Service B depends on service A
Service B is loaded by search extension
Remove service A
Exception: Missing service B

Why?

Services don't even have to come from the same repository so the developer doesn't have control over service A. Service B not being registered instead of reporting that A is missing even though developer configured search extension to find it is really confusing behavior.

I am aware of this feature and was digging in Nette internals for 6 years now and I would still be lost if this happened to me.

@h4kuna
Copy link
Contributor Author

h4kuna commented Jun 21, 2024

@mabar Thank you for explain

I tried your example

class A
{
	public function __construct()
	{
	}
}

class B
{
	public function __construct(private A $a)
	{
	}
}
  • run, ok
  • remove A

Both classes are loaded by search extension and I get exception:
Nette\DI\ServiceCreationException: Service of type Garage\B: Class 'Garage\A' required by $a in B::__construct() not found. Check the parameter type and 'use' statements.

A is regstered, B is loaded in config:
Nette\DI\ServiceCreationException: Service (Garage\A::__construct()): Class 'Garage\A' not found.

B is registered, A is loaded: **Nette\DI\ServiceCreationException:
Service of type Garage\B: Class 'Garage\A' required by $a in B::__construct() not found. Check the parameter type and 'use' statements.

These use cases have legit exception. Nothing about missing Garage\B. But in this case is little bit worse

I revert A and add scalar parameter

class A
{
	public function __construct(string $foo)
	{
	}
}

Nette\DI\ServiceCreationException: Service of type Garage\B: Service of type Garage\A required by $a in B::__construct() not found. Did you add it to configuration file?

The response on question Did you add it to configuration file? is "No because I use search extension", but what happen if I add this class to config? Nette\DI\ServiceCreationException: Service of type Garage\A: Parameter $foo in A::__construct() has no class type or default value, so its value must be specified. It's reason why search engine does not find my class, because I must set up class A before use.

What do you think?

@mildabre
Copy link

mildabre commented Nov 24, 2024

@h4kuna I explain why BC break. I have component FrontMenu rendering simple set of links from array not from database and I pass this component into presenter as service. I register this components by search option - *Menu. This service has no autowired dependency - it would be skipped from search and I must register it manually. Similar problem appear when service use instead of autowiring passing dependency by setter. So it is very big BC break.

But I understand your wish - to have registering services more simple without boilerplate of many hand services registrations or many hand excluded classes. I hate it too.

I use another search system than you - I search only classes by specific class name suffixes like *Model, *Control, *Menu etc. The list becames longer and longer and I loos control over what class is service and what class is not.

To make registering classes as services clear again, I have an idea of implement in DI new feature. Aautomatic registration of services by attribut #[Service] - without writing any code in neon files!

#[Service]
MyClass {.....}

Such a code is absolutely clear, without any doubts, any questions - is this class properly registered or not , or is this class service or is it not service ?? Inventing such attribute would solve much more better your problem and would be NO BC.

@h4kuna
Copy link
Contributor Author

h4kuna commented Nov 24, 2024

@mildabre

This service has no autowired dependency - it would be skipped from search and I must register it manually.

This is not true. If the class has no dependencies, it is registered by default behavior.

Similar problem appear when service use instead of autowiring passing dependency by setter. So it is very big BC break.

No, if you want to use setter, you must define class in neon, in this moment the behavior is same.

I want only extends the excluded classes by automatic. It is mean the dependencies are not defined, like scalar parameter or missing object.

@mildabre
Copy link

mildabre commented Nov 27, 2024

@h4kuna

May be I do not understand 100% what you mean.

"No, if you want to use setter, you must define class in neon, in this moment the behavior is same."

I can define service in search by suffix:

search:
components:
in: '%appDir%/Components'
classes:
- *Control

    model:
     in:  '%appDir%/Model'
     classes:
        - *Model  

Component:

class MyControl
{
private AbstractModel $model;

public setModel(AbstractModel $model): void
{
    $this->model = $model;
}

}

Model class used by component:

class MyModel extends AbstractModel
{
// ...
}

Use in presenter:

public function __construct(
private MyControl $myControl,
private MyModel $myModel,
)
{}

public function actionDoSomething(): void
{
$this->myControl->setModel($this->myModel) // hand injection using setter but out of DI\Container
}

This scenario you must use when you use MyControl in multiple presenters with different models. In this case configuration creates by suffix service without any knowledge about its dependency. I guess that your proposal would exclude MyControl out of services.

The other point to consider is, that you can use search to register services without any dependency intentionally. These services would also be skipped.

@h4kuna
Copy link
Contributor Author

h4kuna commented Nov 28, 2024

@mildabre
Did you try your an example in reality or do you want only theoretically to find a bug? Because I try it and works!

@dg dg force-pushed the master branch 2 times, most recently from 8655bcb to 59cf699 Compare December 2, 2024 05:13
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

Successfully merging this pull request may close these issues.

5 participants