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

Allows entities to be extended by a custom parent class #12

Merged
merged 8 commits into from
Jun 24, 2024
Merged

Allows entities to be extended by a custom parent class #12

merged 8 commits into from
Jun 24, 2024

Conversation

Prometee
Copy link
Contributor

@Prometee Prometee commented Jun 21, 2024

This PR is allowing to extends the 2 Doctrine entities available.

  • Swicth to XML mapping (not required but it will if this plugin wants to support PHP 8.1 one day).
  • Make properties protected instead of private.
  • Switch from Doctrine Table to MappedSuperClass to allow overrides.
  • Add overrided entities to the flex recipe to test the override ?

Note: I saw some entity properties are nullable but they aren't when you look at the database schema definition.

Copy link
Member

@jacquesbh jacquesbh left a comment

Choose a reason for hiding this comment

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

LGTM

@jacquesbh
Copy link
Member

jacquesbh commented Jun 21, 2024

@Prometee The null properties are correct. We use them to avoid issues with Doctrine, since you can't access a property which hasn't been set in the first place. null becomes then the default value. But everything in the code disallows those values to be null elsewhere out of the PHP class.

Copy link
Member

@delyriand delyriand left a comment

Choose a reason for hiding this comment

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

LGTM

And I think we will leave for later the last point of the TODO concerning the entity override test.

@Prometee
Copy link
Contributor Author

@lanfisis or @delyriand Can you run one last time the build to see if everything is ok ?

@lanfisis
Copy link
Member

@lanfisis or @delyriand Can you run one last time the build to see if everything is ok ?

Done, it's running

@maximehuran
Copy link
Member

Thank you @Prometee the doctrine is not up to date, if I run a diff :

        $this->addSql('ALTER TABLE mbiz_alert_message CHANGE enabled enabled TINYINT(1) DEFAULT true NOT NULL, CHANGE customers_only customers_only TINYINT(1) DEFAULT false NOT NULL, CHANGE created_at created_at DATETIME NOT NULL, CHANGE template_html template_html VARCHAR(255) DEFAULT NULL');

It seems the default value is true instead of 1 and false instead of 0.

@Prometee
Copy link
Contributor Author

@maximehuran that's what I saw to, I just fixed it 😉

@maximehuran
Copy link
Member

It stills have

CHANGE created_at created_at DATETIME NOT NULL, CHANGE template_html template_html VARCHAR(255) DEFAULT NULL

@Prometee
Copy link
Contributor Author

Prometee commented Jun 24, 2024

@maximehuran my bad I did not scroll right 😅
I fixed all types it should be good now, you can re-run the build now.

@maximehuran
Copy link
Member

Thank you very much @Prometee 🫶

@maximehuran maximehuran merged commit 5e2c450 into monsieurbiz:master Jun 24, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants