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

[make:entity] Property types, Types:: constant & type guessing #1147

Merged

Conversation

weaverryan
Copy link
Member

Hi!

This expands on the excellent work in #1039. This adds to make:entity:

A) Property types - e.g. private ?int $id = null. Most property types are nullable and default to null, effectively making them typed, but identical to how things worked before (before types, properties were naturally nullable and defaulted to null).

B) Removes most type: options in ORM\Column as these are now "guessed" since doctrine/orm 2.9 from the property types.

C) For the few type: options that remain, we use a Types:: constant.

Cheers!

@weaverryan weaverryan changed the title [WIP][make:entity] Property types, Types:: constant & type guessing [make:entity] Property types, Types:: constant & type guessing Jul 12, 2022
@weaverryan
Copy link
Member Author

This is ready! The Symfony 6.2.x-dev failures are from an upstream bug - symfony/symfony#46916

Copy link
Collaborator

@jrushlow jrushlow left a comment

Choose a reason for hiding this comment

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

Shweet! Couple of minor comments...

src/Doctrine/DoctrineHelper.php Outdated Show resolved Hide resolved
Comment on lines -51 to +50
[Mapping::class => 'ORM'],
['Doctrine\\ORM\\Mapping' => 'ORM'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this for code readability?

src/Util/ClassSourceManipulator.php Outdated Show resolved Hide resolved
src/Util/ClassSourceManipulator.php Show resolved Hide resolved
@jrushlow jrushlow added Feature New Feature Status: Reviewed Has been reviewed by a maintainer labels Jul 12, 2022
Copy link
Collaborator

@jrushlow jrushlow left a comment

Choose a reason for hiding this comment

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

Awesome Work! Thanks @Geekimo && @weaverryan

@jrushlow jrushlow merged commit 63c52a1 into symfony:main Jul 12, 2022
@weaverryan weaverryan deleted the feat/attributes-columns-types-as-constants branch July 12, 2022 17:35
@Geekimo
Copy link
Contributor

Geekimo commented Jul 12, 2022

Thanks @weaverryan for updating my work !

@icedevelopment
Copy link

@Geekimo @weaverryan first off, nice work! I noticed this PR explicitely adds = null after each property which doesn't seem necessary as they should already default to null or maybe I'm missing something?

@Geekimo
Copy link
Contributor

Geekimo commented Oct 28, 2022

@icedevelopment Hello, thanks.
A property never defaults to null unless it is explicitely specified.
If you do not set a default value, it won't be accessible unless initialized via an affectation.

@jorismak
Copy link

jorismak commented Nov 9, 2022

phpstan now always flags the properties as incorrect, because the database says 'not null' and the PHP code says 'nullable'.

I know that you can't access an uninitialized property, but forcing it to null... is that right?

We've worked for years with non-nullable properties this way. You set them in the constructor if you want safe, or other coding standards..

I'm now in doubt if I need to ignore the errors in phpstan, or adjust the scaffolding from make:entity to make the nullable types match the database layout.

Maybe from my C/C++ background, but an uninitialized property (and the dangers of it :P) sound perfectly fine in my head :).

javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this pull request Jan 28, 2023
…ntity output (alamirault)

This PR was merged into the 5.4 branch.

Discussion
----------

[Doctrine] Update example to match actual make::entity output

From slack

![image](https://user-images.githubusercontent.com/9253091/215199944-ecc4544f-62c8-435d-928d-e219b1c5e732.png)

It's a new behavior since maker 1.44.0 released July 26th, 2022
symfony/maker-bundle#1147

(New example is based on local execution)

Commits
-------

32d50e9 Update example to match actual make::entity output
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants