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

Normalization of Doctrine Entities #89

Open
TomHAnderson opened this issue Jun 22, 2020 · 7 comments
Open

Normalization of Doctrine Entities #89

TomHAnderson opened this issue Jun 22, 2020 · 7 comments

Comments

@TomHAnderson
Copy link
Contributor

TomHAnderson commented Jun 22, 2020

I made several suggestions to improve this project in our discourse but now I'd like to offer a new suggestion. Entities often take parameters when created which are other entities which I assume are required by the new entity. I want to change all your entities so none of them take any parameters in the constructor.

This idea runs contrary to dependency injection, I know. My end goal is to enable your application to work with bare-bones entities generated by the Doctrine command line orm:generate-entities. If your entities require the current parameters then those fields should be set to NOT NULL in the metadata and database. This will ensure they must be set.

Doing this will not be difficult. By finding every instance where an entity with a constructor parameter exists then changing it so the next line sets the values on the entity using the entity setters will cause the same affect as passing the parameter. Because it is only constructor parameters only new entities will be affected (because Doctrine creates your entities using doctrine/instantiator and avoids the constructor when fetching database data).

I think this should be done one-entity-at-a-time with each entity getting it's own PR only after the previous entity PR has been merged.

I do not expect nor am I trying to reach my end goal with this proposal; it is a stepping stone along the way.

@davidmintz
Copy link
Owner

Thanks for all your suggestions including this one. By my count all but five entities have no-arg constructors so it should indeed be reasonably easy. But -- forgive my ignorance -- what is the advantage of working with entities generated by orm:generate-entities? The entity classes already exist (obviously), so have no need to be generated.

@TomHAnderson
Copy link
Contributor Author

TomHAnderson commented Jun 23, 2020

I'm happy to teach. You may know I'm on the Doctrine team and I'm responsible for doctrine-module, doctrine-orm-module, and all other Laminas modules from Doctrine. I also helped write Apigility and I wrote the Doctrine in Apigility integration.

Like I said this proposal is just a stepping stone. My plan for your project is to

  • create the database using the Schema Tool
  • reverse engineer the entities into Skipper
  • export the Skipper metadata to XML
  • delete your existing entities
  • generate new entities
  • create the database again using the XML metadata using the Schema Tool
  • compare the two databases until the database generated from the XML is identical to the database generated by the annotated entities.

You see, entities are code you should NOT need to maintain. Using generated entities takes all the worry about all that code out of the project and away from the developer. Now, in a perfect world we'd use only generated entities but it's often the case there are custom functions which belong in an entity. But that's ok because once you generate entities you can start to modify them and when regenerated the modifications will remain. But currently your entities are huge blocks of code and annotations. I want to strip them to bare-bones.

Once all this works then the Skipper ERD is the document of record for the application and all changes to the ORM and database descend from it. You may think, wait, is this a sales pitch? I assure you I don't make any money from the use of Skipper. I just know it's the best product there is. And if once this process is done you are not also convinced then you can continue by manually maintaining the XML metadata files, but I advise against that.

@davidmintz
Copy link
Owner

As I replied in greater detail off the record, for the sake of reducing clutter:

All very interesting and exciting stuff.[...]
Currently my primary focus is per force on responding to bug reports and feature/tweak requests from users. This thing went live on April 25 and a few defects have been exposed -- some of them trivial, sub-one-minute fixes, others not. And there's an important feature that is incomplete, I'm kind of up against a July 31 deadline for that (as a team of one). So I have to devote time to other purposes at the moment[...]

@TomHAnderson
Copy link
Contributor Author

Only you will be fixing bugs, etc. I was offering to offload work so you can have a wider development web.

@davidmintz
Copy link
Owner

Not even sure what "wider development web" means, but it sounds great. Your contributions will be gratefully received.

@TomHAnderson
Copy link
Contributor Author

By wider development web I ment more than just yourself working on the project. I took your post to mean you were not interested in this idea. Can you reply to my post then or was your last reply actually what you intended for a reply?

@davidmintz
Copy link
Owner

Sounds great. Your contributions will be gratefully received.

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

No branches or pull requests

2 participants