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

Nette DI: suspicious dumping of objects when generating the container #306

Closed
Gappa opened this issue Feb 6, 2024 · 8 comments
Closed

Comments

@Gappa
Copy link

Gappa commented Feb 6, 2024

Version: 3.2.0

Bug Description

User Notice
Nette DI: suspicious dumping of objects TestExtensionConfigDTO when generating the container

Steps To Reproduce

final class TestExtensionConfigDTO
{
	// empty class is enough 
}


final class TestExtension extends \Nette\DI\CompilerExtension
{
	public function getConfigSchema(): Schema
	{
		return Expect::from(new TestExtensionConfigDTO);
	}
}
extensions:
	- TestExtension

Expected Behavior

No error message is shown or a message that is better at explaining what is suspicious about the object/class and how to fix it.

Possible Solution

N/A


I'm not really sure what to do about this or what actually is the problem - I'm using objects like this:

Edit - relevant commit:

@dg
Copy link
Member

dg commented Feb 6, 2024

It looks like the TestExtensionConfigDTO object can't be simply created, so it is created in the container using Dumper::createObject(), i.e. using serialization+unserialization, which is a pretty dirty way.

https://github.com/nette/php-generator/blob/08ab9bff22ae34fe4e1d2fe8ba16b3770ea2459f/src/PhpGenerator/Dumper.php#L290-L297

But I understand why you use it that way, let's try to find a solution.

The solution I can think of is that TestExtensionConfigDTO will have a constructor (and all properties will be promoted parameters), so such an object can be easily created. But that's not enough, some information will still have to be added for the DI compiler to use that constructor…

@mabar
Copy link
Contributor

mabar commented Feb 6, 2024

I am confused what does it have to do with DI compiler. Isn't config schema used just to access config values of the extension? They are not passed to generated DIC

For objects that are passed to generated DIC, I am using explicit serialization, which is fine as long as changing the object forces DIC to regenerate (can be done by adding object to compiler dependencies) https://github.com/orisai/localization/blob/ae99720c58b10408b099e68c9692f75a45c4b627/src/Bridge/NetteDI/LocalizationExtension.php#L131-L136

@dg
Copy link
Member

dg commented Feb 6, 2024

@mabar I suppose @Gappa will then pass the $this->config to some service. Right?

@Gappa
Copy link
Author

Gappa commented Feb 6, 2024

@mabar I suppose @Gappa will then pass the $this->config to some service. Right?

Yes, that is correct.

@janfejtek
Copy link

I think having some better decision making when triggering error would be fine? Plus some changes in Dumper::createObject logic to avoid unserialization. Maybe some similar logic to form values? https://github.com/nette/forms/blob/d42a22b451fcdf267e7c485309370506a8731d56/src/Forms/Container.php#L137

If I have simple class like this

class Data {
   public string $url;
   public string $query;
}

I would say it is really close to \stdClass, but it is not accepted
c257fa9#diff-b01fda112fb0df465a70dd098f3843dd90fc3a0ce514c37ef38d620e7f7cc718R184

@dg
Copy link
Member

dg commented Feb 8, 2024

I don't know how to dump this object:

class Data {
   public string $url;
   public string $query;
}

but I can dump this one:

class Data {
  function __construct(
   public string $url,
   public string $query,
  ) {}
}

@dg dg closed this as completed in 6b9910c Feb 12, 2024
@Gappa
Copy link
Author

Gappa commented Feb 13, 2024

Tried it with 6b9910cece1e074ad1d1babc6a62c48dfa9ea870 and it works now - thanks. 👍

However, would it be better if I changed those DTOs to classes with private properties and a constructor?

@dg
Copy link
Member

dg commented Feb 13, 2024

@Gappa I'm not sure yet, I'll let you know

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

4 participants