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

Container::$parameters contains unresolved Statement instances #221

Closed
xificurk opened this issue Oct 26, 2019 · 6 comments
Closed

Container::$parameters contains unresolved Statement instances #221

xificurk opened this issue Oct 26, 2019 · 6 comments

Comments

@xificurk
Copy link
Contributor

@xificurk xificurk commented Oct 26, 2019

Version: dev-master, 3.0.x, 2.4.x

Bug Description

Parameters attribute in generated DI container contain unresolved Nette\DI\Definitions\Statement instances.

Steps To Reproduce

Generate container with the following config:

parameters:
	bar: ::getenv(FOO)

The generated code of DI container will have something like this in constructor:

$this->parameters += [
	'bar' => Nette\PhpGenerator\Helpers::createObject('Nette\DI\Definitions\Statement', [
		'arguments' => ['FOO'],
		"\x00Nette\\DI\\Definitions\\Statement\x00entity" => ['', 'getenv'],
	]),
];

Expected Behavior

I would expect the parameters to be resolved the same way as they are if they are used as arguments in service definitions.

@xificurk
Copy link
Contributor Author

@xificurk xificurk commented Oct 31, 2019

Thank you ❤️

@dg
Copy link
Member

@dg dg commented Jan 7, 2020

I decided to remove this feature.

Resolving parameters takes time and performance, it can instantiate services, and so on, but nobody, in fact, uses parameters. Instead, I'll remove unresolved statements from code and it will be cleaner.

dg added a commit that referenced this issue Jan 7, 2020
This reverts commit c1a0261.

Resolving parameters takes time and performance, services can be created, but nobody uses parameters.
dg added a commit that referenced this issue Jan 8, 2020
This reverts commit c1a0261.

Resolving parameters takes time and performance, services can be created, but nobody uses parameters.
@dakujem
Copy link

@dakujem dakujem commented Jan 8, 2020

A problem I had with Nette 2.x was that once you do actually want to quickly pull a parameter out of configuration, say for prototyping, it is not possible, because instead of a value, one gets a Statement object.
If it was possible to fetch the underlying value or "resolve" all or a subset of parameters, it would be awesome.
Something like the Resolver::completeArguments, that we could call on the container.

The usecase would be this:

$raw = $presenter->context->getParameters()['foo'];
$value = Something::resolve($raw);

// or
$value = $presenter->context->resolveParameter('foo');

// or
$presenter->context->resolveParameters();
$value = $presenter->context->getParameters()['foo'];

... I bet you get the idea, David @dg

I am aware that the use case is not a good practice, yet this does happen and is especially frustrating for people who have not mastered Nette, for example a React developer who just wants to quickly add this tiny parameter to the API for quick proof of concept...

@dakujem
Copy link

@dakujem dakujem commented Jan 8, 2020

Resolving parameters takes time and performance

👍

@dg
Copy link
Member

@dg dg commented Jan 8, 2020

What about this way?

class Parameters
{
	private $params;

	function __construct(array $params)
	{
		$this->params = $params;
	}

	function get($key)
	{
		return $this->params[$key]
	}
}

Add it as service:

parameters:
	main:	
		a: ::getenv('xxx')
		b: xzy

service:
	- Parameters(%main%)

And then use $params = $presenter->context->getByType(Parameters::class); and $params->get('a')

@dakujem
Copy link

@dakujem dakujem commented Jan 8, 2020

I am injecting the parameters in groups, right.
A service like the oabove would "unpack" (evaluate) all the parameters, possibly not optimal for performance.

Anyway, this technique (injecting) is not something a newbie will come out with, trust me :-) I have been asked this over and over again ("How do I convert a Statement into a value?").

dg added a commit that referenced this issue Jan 10, 2020
This reverts commit c1a0261.

Resolving parameters takes time and performance, services can be created, but nobody uses parameters.
dg added a commit that referenced this issue Jan 20, 2020
This reverts commit c1a0261.

Resolving parameters takes time and performance, services can be created, but nobody uses parameters.
markoph added a commit to remp2020/crm-skeleton that referenced this issue Mar 31, 2022
Environment variable `CRM_LANG` was removed.

---

Problem:

- [Kdyby/Translation expects default language to be string type][1].
- From version 3.0, `Nette/DI` returns `Nette\DI\Definitions\Statement`
  instead of string (it used to evaluate method used in config; in our
  case `@environmentConfig::get('CRM_LANG')`).
- `Nette\DI` returns error

  ```
  Nette\DI\InvalidConfigurationException:
  The item 'translation › default' expects to be string, object
  Nette\DI\Definitions\Statement given.` is returned.
  ```

This worked in Nette 2.4 because `Nette/DI` resolved parameters (in this
case it called method to load environment variable) and it returned string
to extension DI compiler.

---

Solution: Removed loading of language from environment (variable
CRM_LANG was removed).

If you run different instances from one code base, it's better to have
different `config.*.neon` files. Which config is loaded is based on env
variable `CRM_ENV`. Check `Crm\ApplicationModule\Core::createContainer`.

---

Note: We checked alternative extension, but [Contributte/Translation also
expects string parameter][2].

---

Discussion:

- [Temporarily fixed, but reverted back][3]
- [Options in DI extensions do not expect Statement instances][4]

[1]: https://github.com/Kdyby/Translation/blob/4c4a02a22922b8d69961bb4cd17c104c451622a4/src/DI/TranslationExtension.php
[2]: https://github.com/contributte/translation/blob/e78977eb2ce0cc3104ce1fa880516521cdef228c/src/DI/TranslationExtension.php
[3]: nette/di#221 (comment)
[4]: nette/di#228

remp/crm#2144
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants