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

[Translatable] Ignore notInsertable columns… #2836

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dgrothaus-mc
Copy link

in postPersist hook.

Respect the column's metadata and don't attempt to insert a column that's flagged to be not-insertable.

When a custom Translation class configures a field that is flagged as not insertable

#[ORM\Column(
    insertable: false
)]

then don't attempt to insert the field in the postPersist hook.

While this works with MySql and MariaDB, inserting a generated column in Postgres throws an error.

cannot insert a non-DEFAULT value

@mbabker
Copy link
Contributor

mbabker commented Jul 2, 2024

Would you be able to add unit tests covering this change?

Also, the ODM part of this needs an update. The #[Field] attribute uses a notSaved property (with that being the key written to the mapping array) compared to the ORM's insertable property and notInsertable mapping key.

@dgrothaus-mc
Copy link
Author

Would you be able to add unit tests covering this change?

Also, the ODM part of this needs an update. The #[Field] attribute uses a notSaved property (with that being the key written to the mapping array) compared to the ORM's insertable property and notInsertable mapping key.

Sure. I'll add a unit test and force-push the branch with the changes to ODM.

@dgrothaus-mc
Copy link
Author

Sorry for coming back to you so late. I've been OOO for longer than anticipated.

Could you give me a hint on how to trigger the test for the ODM part?
The ORM part was quite easy. As there's already a test with a custom Translation class https://github.com/doctrine-extensions/DoctrineExtensions/blob/main/tests/Gedmo/Translatable/Fixture/PersonTranslation.php I could add a field to that class to trigger the ORM part of the change.

As far as I can see, there's currently no existing ODM test for a custom Translation Document, that extends https://github.com/doctrine-extensions/DoctrineExtensions/blob/main/src/Translatable/Document/Translation.php
Should I just add a completely new test with all classes required to use a custom Translation Document?

@mbabker
Copy link
Contributor

mbabker commented Jul 29, 2024

Should I just add a completely new test with all classes required to use a custom Translation Document?

If you can, that'd be great. But if it's all too complex (there is a lot of stuff in this package only tested through the ORM), then we can come back around to improving the test coverage later.

It looks like there's already a TranslatableDocumentTest and some fixtures in https://github.com/doctrine-extensions/DoctrineExtensions/tree/main/tests/Gedmo/Translatable/Fixture/Document so maybe all we need is a new test case in that class and whatever fixture updates are involved.

in postPersist hook.

Respect the column's metadata and don't attempt to insert a column
that's flagged to be not-insertable.
Adding this field to the test suit triggers the ignored flag for
Doctrine fields.
Adding a unit test to test custom ODM translation repositories, as there
already exist for ORM.
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.

3 participants