doctrine-extensions
[SoftDeleteable] Make deleted value configurable for SoftDeleteableFilter.php
@phansys We have added some tests like the existing ones, does this meet the requirements?
You mind describing the use case here? Also, just on a glance over this PR it seems like a lot more changes than this would be needed considering the extension is designed around using nullable date columns for the deleted value, so widening it to support non-date values in place of null feels like it would need more changes.
You've also only updated the ORM's filter class, the ODM's should be updated as well to keep both implementations working the same way.
As for the tests, new test fixtures should be added which cover the new configuration option and not just adding it to the existing tests.
The use case is as follows: Mysql: "UNIQUE index permits multiple NULL values for columns that can contain NULL." https://dev.mysql.com/doc/refman/8.4/en/create-index.html (which any soft delete column currently must be) If there is a case where one would like to only allow one not-softdeleted value, this is currently impossible with softdelete. Allowing a different value than null to be set to be seen as not-deleted for the softdelete column would permit allowing a not-deleted softdeletable entries to be unique.
@mbabker Do you consider new test satisfactory? Otherwise I'd like some guidance on what exactly you'd like to see tested and how.
@phansys We'd love to get this merged, any help you can provide to help us to reach that point would be much appreciated.
@phansys @mbabker Hello, we assume you guys have a backlog with items to work through, but could you give an indication on how / when we can proceed? Thank you in advance :)
I don't have merge rights so I can't do anything with this. But based on the current state of the PR:
- I'd revert the changes in the
tests/Gedmo/Mapping/Driver/Xml/Gedmo.Tests.Mapping.Fixture.Xml.SoftDeleteable.dcm.xml,tests/Gedmo/Mapping/Driver/Yaml/Gedmo.Tests.Mapping.Fixture.Yaml.SoftDeleteable.dcm.yml, andtests/Gedmo/Mapping/Fixture/SoftDeleteable.phptest fixtures so those continue to test the default behaviors (which would also imply reverting the changes inSoftDeleteableMappingTest) - For mapping tests, we'd want new fixtures with the new config option to validate the mapping part of things (a lot of the fixtures are just copy/paste classes and adapted to different config scenarios)
- The added test case in
SoftDeleteableEntityTestis good, I'd add a similar case for theSoftDeleteableDocumentTestto cover the MongoDB part of this (but do double check the added fixtures to make sure the annotations and attributes are consistent, theUserNonDeletedValuefixture has different configs so the test builds using annotations are going to have different behaviors from the builds using attributes)
My one outstanding concern is that the current state of the fixtures (especially the #[Gedmo\SoftDeleteable(fieldName: 'deletedAt', nonDeletedColumnValue: '0')] attribute) implies that this should support non-datetime values. If that's intended, then there should be at least one test fixture and case in the document and entity tests to ensure this is working. If it's intended for this to only support datetime-ish values or null, then there should probably be a check for this in the mapping drivers so users don't misconfigure things and file bug reports because things aren't working with bad configs in place.
@mbabker Thank you for the clear feedback, I have made the requested changes.
It is indeed intended to work with datetime-ish or null values, and I have updated everywhere 0 was used to be DateTime instead. There is already Validator::validateField in the mappers that validates that only datetime-ish values are used.
@phansys It would be greatly appreciated if this could be merged.