8000 [PropertyAccess] Auto-cast from/to DateTime/Immutable when appropriate by nicolas-grekas · Pull Request #50295 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[PropertyAccess] Auto-cast from/to DateTime/Immutable when appropriate #50295

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 13, 2023
Merged

[PropertyAccess] Auto-cast from/to DateTime/Immutable when appropriate #50295

merged 1 commit into from
Jul 13, 2023

Conversation

@nicolas-grekas
Copy link
Member
@nicolas-grekas nicolas-grekas commented May 10, 2023 •
edited
Loading
Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR -

Best reviewed ignoring whitespaces.

My starting point is a form with a date defined as such:

class MyFormType extends AbstractType {
    public function buildForm(FormBuilderInterface $builder, array $options): void
    {
        $builder
            ->add('some_date', DateTimeType::class)
        ;
    }
// ...
}

class MyEntity
{
    private ?\DateTimeImmutable $someDate = null;
//...
}

Then doing something like:

$myEntity = new MyEntity();
$form = $this->createForm(MyFormType::class, $myEntity);

If I submit this form, I get:

Expected argument of type "DateTimeImmutable", "DateTime" given at property path "some_date".

The current solution is to set option input to datetime_immutable in buildForm().

But this is boring. Too much boilerplate.

I've tried looking at the Form component to define the input option based on the entity provided when building the form, but this entity is not passed to sub-forms, so that we have no way to do this.

The only way I found is the one attached: if we cannot set a DateTime, we try setting a DateTimeImmutable.

I thought about changing the default value of option input to datetime_immutable, but this would need a nasty deprecation that'd force everybody to write even more boilerplate before v7, while what we want is less.

So here we are.

@carsonbot carsonbot added this to the 6.3 milestone May 10, 2023
@symfony symfony deleted a comment from carsonbot May 10, 2023
@ro0NL
Copy link
Contributor
ro0NL commented May 10, 2023

would the try/catch be possible on the calling side?

@nicolas-grekas nicolas-grekas mentioned this pull request May 11, 2023
Merged
@stof
Copy link
Member
stof commented May 11, 2023

If we go this way, I suggest supporting the auto-cast in both directions. This would make it easier to change the default input mode in the Form component in a future version.
In 7.0, the Form component should probably be changed to use DateTimeImmutable is its norm data and converting to DateTime when the input needs it rather than working with mutable DateTime for all intermediate layers. But this would make sense if we don't convert twice for projects using DateTimeImmutable and the default input format.

@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
fabpot added a commit that referenced this pull request Jun 3, 2023
@fabpot
minor #50297 Remove unnecessary usages of DateTime (nicolas-grekas)
044b344 
This PR was merged into the 6.4 branch.

Discussion
----------

Remove unnecessary usages of DateTime

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | Part of #47580
| License       | MIT
| Doc PR        | -

Together with #50290, this PR removes `DateTime` everywhere possible.

What remains is:
- test cases that test both DateTime and DateTimeImmutable - legit to keep
- date/time form-types and transformers in the Form component - we'd need a separate effort for them, related to #50295
- `PersistentTokenInterface::getLastUsed(): \DateTime` - separate effort also

Commits
-------

8b08294 Remove unnecessary usages of DateTime
@nicolas-grekas nicolas-grekas changed the title [PropertyAccess] Auto-cast from DateTime to DateTimeImmutable when appropriate [PropertyAccess] Auto-cast from/to DateTime/Immutable when appropriate Jun 6, 2023
@nicolas-grekas
Copy link
Member Author
nicolas-grekas commented Jun 6, 2023 •
edited
Loading

would the try/catch be possible on the calling side?
supporting the auto-cast in both directions

Done for both.

@nicolas-grekas
Copy link
Member Author
nicolas-grekas commented Jun 21, 2023

Now with tests. PR ready for review.

@fabpot
Copy link
Member
fabpot commented Jul 13, 2023

Thank you @nicolas-grekas.

@fabpot fabpot mentioned this pull request Jul 13, 2023
Closed
@fabpot fabpot merged commit e868f0a into symfony:6.4 Jul 13, 2023
@nicolas-grekas nicolas-grekas deleted the pa-cast-date branch July 25, 2023 15:45
@nicolas-grekas nicolas-grekas mentioned this pull request Aug 1, 2023
Closed
@curry684
Copy link
Contributor
curry684 commented Aug 1, 2023

Good that the feature is in, but you're now relying on exceptions for normal program flow. The approach I followed in #47776 does not, and may therefore be more performant I believe?

@nicolas-grekas
Copy link
Member Author
nicolas-grekas commented Aug 1, 2023

Could you run some bench? The reflection logic might be quite involving also, especially considering using types/etc

@curry684
Copy link
Contributor
curry684 commented Aug 1, 2023

Also I made some specific exceptions:

Coercion was set up to be more extensible for future similar cases, and will only be attempted for method-based and public access. Coercing values for adder/remover makes no sense as it would (nearly) always break the remover functionality. Likewise coercion is specifically not attempted for union/intersection types, only for explicitly named types.

Your implementation is not excluding adder/remover (which may cause collections to end up with a rag tag mix of all types) nor union/intersection types.

This was referenced Oct 21, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fabpot fabpot fabpot approved these changes

@xabbuh xabbuh Awaiting requested review from xabbuh

@yceruto yceruto Awaiting requested review from yceruto

Assignees

No one assigned

Labels

Feature PropertyAccess Ready Status: Reviewed

Projects

None yet

Milestone

6.4

Development

Successfully merging this pull request may close these issues.

6 participants

@nicolas-grekas @ro0NL @stof @fabpot @curry684 @carsonbot
0