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 to set a null template #6196

Merged
merged 1 commit into from
Jul 17, 2020

Conversation

VincentLanglet
Copy link
Member

Subject

Targeting this branch, because the typehint was added in master.

SonataDoctrineOrmBundle call this setter with a possible null value.
Since the template is null by default, I updated the phpdoc and I don't see any problem with allowing to set null.

@phansys
Copy link
Member

phansys commented Jul 13, 2020

SonataDoctrineOrmBundle call this setter with a possible null value.

Maybe we could prevent this method to be called in that case.
I generally prefer to reserve the values as null only for the initial class state, so the setters has no possibility to set null in case a previous value was provided. Surely I have to re-analize this behavior since PHP 7.4, where the property declaration makes the initialization concept more explicit.

Copy link
Member

@wbloszyk wbloszyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirm, this working.

@VincentLanglet
Copy link
Member Author

SonataDoctrineOrmBundle call this setter with a possible null value.

Maybe we could prevent this method to be called in that case.

Yes we could write

if (null === $field->getTemplate()) {
    $computed = $this->foo();
    if (null !== $computed) {
        $field->setTemplate($computed);
    }
}

But

if (null === $field->getTemplate()) {
    $field->setTemplate($this->foo());
}

was more convenient.

And most of the setters of this class are allowing null as a possible value.
So I see no reason for a different treatment.

I generally prefer to reserve the values as null only for the initial class state, so the setters has no possibility to set null in case a previous value was provided.

Setting null allow to reset the value. I don't see reason to forbid this in this case.

@phansys
Copy link
Member

phansys commented Jul 13, 2020

Setting null allow to reset the value. I don't see reason to forbid this in this case.

In my case I think it's just a personal preference. I haven't strong reasons against this behavior, just wondering why we should accept a value to be set and later to be resetted. IMO, disallowing the possibility to reset makes the class state more robust. I'm thinking about cases like the Request class.

@VincentLanglet VincentLanglet requested a review from a team July 14, 2020 15:10
@VincentLanglet
Copy link
Member Author

Setting null allow to reset the value. I don't see reason to forbid this in this case.

In my case I think it's just a personal preference. I haven't strong reasons against this behavior.

Is it ok for you if I merge this then ?

@phansys
Copy link
Member

phansys commented Jul 14, 2020

Is it ok for you if I merge this then ?

I'm sorry, but I'd like to abstain from this vote in order to let other contributors to decide about making this kind of changes deliberately.

@wbloszyk
Copy link
Member

IMHO FieldDescription working on specify AdminBundle scope. SonataDoctrineOrmBundle is extension and should override template for something better(other). Null value is taking from guesser, it can be fix by easy:

if ( $newTemplate = $this->guessType($field)) {
Set it
}

Sorry but i write it on my phone

@VincentLanglet VincentLanglet merged commit 43b306d into sonata-project:master Jul 17, 2020
@VincentLanglet VincentLanglet deleted the fixType branch July 17, 2020 06:48
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