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

Add options in listeners: forceUseAttributeReader and separateXmlMapping #2657

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

eisberg
Copy link

@eisberg eisberg commented Jul 27, 2023

Closes #2613 and #2318

1. Mapping Driver: Configure usage separately with Doctrine .orm.xml -> .gedmo.xml files

Thanks to this option, we can transfer options related to setting up Gedmo properties from a common configuration file with the doctrine to a separate EnittyName file.gedmo.xml !

$blameableListener = new BlameableListener();
$blameableListener->setSeparateXmlMapping(true);

Example MyEntity.gedmo.xml:

<?xml version="1.0" encoding="utf-8"?>
<doctrine-mapping xmlns="http://doctrine-project.org/schemas/orm/doctrine-mapping"
                  xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                  xsi:schemaLocation="http://doctrine-project.org/schemas/orm/doctrine-mapping https://www.doctrine-project.org/schemas/orm/doctrine-mapping.xsd"
                  xmlns:gedmo="http://gediminasm.org/schemas/orm/doctrine-extensions-mapping"
>
    <entity name="MyEntity">
        <field name="lastEditedBy" >
                <gedmo:blameable on="update"/>
        </field>
    </entity>
</doctrine-mapping>

Default value: FALSE

2. Alternative option: force the use of AttributeReader and ignore the configuration of the Doctrine chain driver

$blameableListener = new BlameableListener();
$blameableListener->setForceUseAttributeReader(true);
...
# Entity:
    #[Gedmo\Blameable(on: 'create')]
    private $createdBy;

After that, the Gedmo options will be read from the attributes, despite the fact that the basic configuration of the mapping doctrine is read in XML files

Default value: FALSE

@codecov
Copy link

codecov bot commented Sep 21, 2023

Codecov Report

Attention: Patch coverage is 64.86486% with 13 lines in your changes missing coverage. Please review.

Project coverage is 79.62%. Comparing base (0632ab1) to head (f5bd260).
Report is 58 commits behind head on main.

Current head f5bd260 differs from pull request most recent head 3b0553f

Please upload reports for the commit 3b0553f to get more accurate results.

Files Patch % Lines
src/Mapping/Driver/File.php 58.82% 7 Missing ⚠️
src/Mapping/MappedEventSubscriber.php 42.85% 4 Missing ⚠️
src/Mapping/ExtensionMetadataFactory.php 84.61% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2657      +/-   ##
==========================================
+ Coverage   78.75%   79.62%   +0.87%     
==========================================
  Files         163      161       -2     
  Lines        8593     8478     -115     
==========================================
- Hits         6767     6751      -16     
+ Misses       1826     1727      -99     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mbabker
Copy link
Contributor

mbabker commented Jun 25, 2024

I have to be honest, the more I think about this PR, the more I'm not sure I agree with it. While the XSD validation issue is a legitimate concern (and one the ORM maintainers took into account before releasing 3.0 by reverting the mandatory validation, and the MongoDB ODM maintainers closely following the relevant ORM issues), I think both ideas proposed in this PR just makes things more complicated from an end user and a maintainer perspective.

One of the biggest benefits to piggybacking off the Doctrine mapping API is all of the problems they've already solved are solved for us. Separating out the mapping (either through separate XML configs or having models with XML definitions and requiring PHP attributes to add this package's functionality) seems like it would re-introduce a lot of those kinds of solved problems. How well does this PR account for models with inheritance (which is a big thing in the Sylius ecosystem where model schemas are defined with XML, and is functionality provided through this package with a number of mapped superclasses)?

If this PR is to move forward, it will need a lot of tests covering this behavior as it is a huge behavioral change if you opt into either of these features.

@eisberg
Copy link
Author

eisberg commented Jun 27, 2024

There is nothing self-written in this solution, only (1) the doctrine XMLReader is launched for separate files OR (2) the standard doctrine AttributeDriver is launched instead of the one installed in the general configuration. Well, besides, when these options are disabled, the standard behavior is preserved. It's just a configuration manipulation of which reader to use.

We have been successfully using the patch from this PR for a year now, in a product that has about 1000 ORM models. Well, this is just a little confirmation that it works.
I will be glad if an alternative and better solution is proposed. Or, if someone takes this PR and writes a lot of tests for this behavior.
But for now, without this patch, we cannot use this module.

Convert all our ORMs to attributes, only because DoctrineExtension does not support XML, or rather, Doctrine forbade writing something third-party, dubious activity in its files.
Sorry, I’m judging from a consumer point of view, and it’s easier for us to abandon this library altogether.

PS: During the year of the known problem, no alternative was proposed

@eisberg
Copy link
Author

eisberg commented Jul 20, 2024

Add other PR: #2845
With only attributes reader.

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.

XML Validation Error with Doctrine 2.14.2
3 participants