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

Allow disabling strict types. #235

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dakujem
Copy link

@dakujem dakujem commented Mar 23, 2020

  • new feature (or maybe a bug fix?)
  • BC break? no

Raw usage:

$c = new Compiler();
$c->setStrictTypes(false);
$code = $c->compile(); // Et voila! No strict types.

With nette/bootstrap in a Nette application:

$configurator = new Configurator();
$configurator->onCompile() = function(Compiler $c){
    $c->setStrictTypes(false);
};
$configurator->createContainer();
// Et voila! No strict types.

@dg
Copy link
Member

dg commented Mar 26, 2020

What is the reason to disable strict types? The container should be generated to respect strict types.

@dakujem
Copy link
Author

dakujem commented Mar 26, 2020

Migration from an old older code base, where type coercion is widely in use.

Anyway, since setting lax types is allowed in Latte, I see no reason to not allow users to do this with the container. A good library is a flexible library.

This PR adds a feature that does not break anything and adds no overhead for any running app.

@dg
Copy link
Member

dg commented Mar 26, 2020

Latte is something different because you write the code of template, while the DI is completely generated.

The goal is for the generator to be able to look at the compatibility of types and avoid errors. So this PR goes against the intentions of the library.

You can describe what specific type coercion errors you have encountered?

@dakujem
Copy link
Author

dakujem commented Mar 26, 2020

Of course.

In a NEON file there is a factory or any other service in fact, that has a type hint bool in the PHP implementation. Load an ENV var to that parameter... Bang!

class FooService {
function __construct(bool $on){ ... }
}
services:
  foo: FooService(::getenv(ENABLE_FOO))

This is an oversimplified example, but it demonstrates the problem. I am aware one can solve this with ::boolval(::getenv(ENABLE_FOO)), but it's a tedious work for the amount of components I'm dealing with. I am also sure that this will lead to dropping the type hints in the PHP implementations in the future (lazy devs, right?).

@dakujem
Copy link
Author

dakujem commented Mar 26, 2020

Let me elaborate on this issue.

In order to enforce correct types in NEON format, one has to use functions boolval, strval, intval, floatval, doubleval. This is not ideal, since in PHP you would typically prefer to use (bool), (str) etc. for type casting. It also adds to the ballast that NEON format tries to get rid of in the first place.
It gets most impractical when using built-in functions like getenv that tend to return multiple types (string or FALSE in case of getenv). There is a multitude of functions that return FALSE on error, including third party libraries.
Also, there is no simple way of using neither ternary operator nor null coalescing operator nor logical operators that would simplify the task of type enforcement in NEON.

I myself am not a big fan of declare(strict_types=1). So why am I forced to use them? It's exactly because the container is fully generated, that this is obtrusing. Let me explain.

In a function like the following, it is obvious that it returns a string and accepts a string.

function foo(string $input): string

So while you implement the function, it is guaranteed that you are given strings, whatever the call argumetns are, even without strict types. Then the caller is also guaranteed that the return value will be a string, whatever the inner implementation might be, again, even without the strict types.
Why should I be screamed on, when I want to return a new StringableObject instance from within the foo function? I know exactly what it does. The output will be a string. So why force me to add the (string) type cast there?
If the caller wishes, he can still declare strict types in that file and make sure the compiler punishes him for not using the correct typy, in his file.

While implementing services, components or factories I don't care how the caller uses them. All I care about are the type-hinted parameters that guarantee what the input will be like.

Now, I understand that the opinions will differ. The thing is though, this is a library and as such it should be made flexible.

@dg
Copy link
Member

dg commented Mar 26, 2020

The use of strict types is not based on boolval, strval, (bool) or (string). On the contrary, these type casting operators and functions go against the meaning of strict types. Well written strictly typed code will almost not use them. So even the configuration file must work without casting.

Because strict types is about preventing and detecting loss of information. Typecasting operators do the opposite.

getenv() is problematic because it only returns strings. If I want to use this function in strictly typed code, it's more complicated, I have to create an intermediate layer. But it is not the problem of that code (or configuration), it is the problem of getenv.

Possible solution is to create functions like

// no strict types
function intenv($name): int 
{
	return getenv($name); 
}

Usage:

putenv('test=123');
echo intenv('test'); // returns integer 123

But

putenv('test=XXX');
echo intenv('test'); // TypeError: Return value of intenv() must be of the type int

It is important that XXX was not quietly converted to zero, but it reported an error. It is point of strict types, to prevent and detect data loss.

Now you can safely use ::intenv() in config file.

@dakujem
Copy link
Author

dakujem commented Mar 26, 2020

Well I am aware of how one should be using strict types.

It comes easily once writing fresh code.

From my perspective though, where I had to migrate a codebase written for N2.4 that relies on type coercion, the strict types in the DIC were a headache. Of course I turned them off. And I would like to allow to the others to do so without having to hack into the configurator as I did spending an hour to find a way to do it "gently".

But I take your point that one should probably invest in making the types safe.
In the case I am dealing with though, as I'm reading ENV vars into configuration, I see no point in differentiating between '1' and 1, or '1' and true, or '' and false and so on, you get the idea.

What are we going to do with this PR?

BTW: your migration guide N2.4 → N3.0 is very optimistic 😊

@dg
Copy link
Member

dg commented May 20, 2020

Btw I added some type conversion functions to DI 2fab800

@dakujem
Copy link
Author

dakujem commented May 21, 2020

Also, "nullable" type conversion would be awesome. And operators like ternary or null-coalescing. Though I don't know how to add those, can't help much there.

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.

None yet

2 participants