use XML instead of annotations for Doctrine mapping to allow overriding it

[craue]

Resolves #19. I've chosen XML over YAML to avoid a new dependency to symfony/yaml.

@Rvanlaak: Could you try this to make sure the mapping can actually be overridden as expected?

[rvanlaak]

I've chosen XML over YAML to avoid a new dependency to symfony/yaml.

The bundle is already relying on symfony/symfony so that dependency already is available

[craue]

The bundle is already relying on symfony/symfony so that dependency already is available

Only as a dev requirement.

[craue]

@Rvanlaak: Fine to merge?

[rvanlaak]

Will check the PR branch later today

[rvanlaak]

Update 1: everything is working fine after switching to the XML annotations.

Will try to override the XML mapping now.

[rvanlaak]

Doctrine tries to drop the UNIQUE index:

DROP INDEX UNIQ_B95BA9425E237E06 ON craue_config_setting;

[rvanlaak]

Next to that, Doctrine won't allow us to override the entity without creating a super-class: http://symfony.com/doc/current/cookbook/bundles/override.html#entities-entity-mapping In other words, moving to XML won't solve this problem.

We should make the config class a parameter, in the way how Sylius allows to override models: http://docs.sylius.org/en/latest/bundles/general/overriding_models.html

[craue]

...or introduce a mapped superclass? I've added a commit for that. I now actually like the idea of being able to provide several mappings (ORM, ODM) and using dedicated files (XML or YAML) for that seems most convenient. A class config parameter could be added, but I guess this PR would still be needed in such case. Anyway, this is all new for me, so don't hesitate to prove me wrong. :smirk:

Doctrine tries to drop the UNIQUE index

Are you sure it's due to this PR? I cannot reproduce that.

[rvanlaak]

A class config parameter could be added,

Without that I don't see a way to let your Bundle use the Entity located in my application. Maybe add some docs about extending the Settings entity with some example code?

but I guess this PR would still be needed in such case.

Definitely! :+1:

[craue]

I made the class configurable and added a test, but there's one thing I don't have a solution for yet: The mapping file (Tests/IntegrationTestBundle/Resources/config/doctrine-mapping/CustomSetting.orm.xml) for the custom (overridden) class needs to be loaded, if one is used. Any idea how to achieve that?

[rvanlaak]

Will pull the changes and take a look into that within the hour :+1:

http://doctrine-orm.readthedocs.org/en/latest/reference/inheritance-mapping.html#inheritence-mapping-overrides

[rvanlaak]

Hmm, did find the "things to note" in this chapter:

http://doctrine-orm.readthedocs.org/en/latest/reference/inheritance-mapping.html#attribute-override

[craue]

Are you pointing to something special? Looks like all this stuff (overriding parts of a model, extending a model, supporting multiple drivers) is not as easy as I thought.

[rvanlaak]

Yeah, the following:

The column type CANNOT be changed. If the column type is not equal you get a MappingException

[craue]

And you want to change the type of the value field, right? :smirk: If so, we cannot use/provide a mapped superclass and have to force users into specifying the complete mapping for the model. I don't like that idea and hope there's another way...

[rvanlaak]

@beberlei is there any chance overriding attribute types will become possible in the near future?

[rvanlaak]

This will probably only work if value is moved from the BaseSetting to the Setting entity. An extra interface is needed to ensure value is implemented.

Then I can determine the way how value looks in my own application, without overriding anything.

[craue]

This will probably only work if value is moved from the BaseSetting to the Setting entity.

I tried that locally but still couldn't get the mapping file loaded for the test.

An extra interface is needed to ensure value is implemented.

What do you mean? SettingInterface already has methods setValue and getValue.

[craue]

If there's no way to make this work, I'm forced to drop that idea completely.

[craue]

@rvanlaak, I started another approach and could finally make it work. 😄 Please take a look.

[rvanlaak]

Oh wow really, that would be awesome! So then we finally can make the config a text instead of a varchar field? ;)

I'm now looking into a nice way to make these configurations translatable / internationalizable :-)

[craue]

I just made one more change to render the built-in form fields as textarea when the overridden entity maps the field value to a text column.

I'm now looking into a nice way to make these configurations translatable / internationalizable :-)

Luckily out of the scope of this PR. 😏

[coveralls]

Coverage decreased (-0.3%) to 95.161% when pulling 4efac976f0708b8de87ed354fda06f4cafe3e86c on doctrine-mapping-xml into 59ce2c0bc7d318bfaddd25de123bcbb905afd68b on master.

[craue]

@rvanlaak, are any further changes needed or do you want me to just push the merge button? 😏

[coveralls]

Coverage decreased (-0.3%) to 95.161% when pulling 3f447e15dd8328673403013682e41862074f1e76 on doctrine-mapping-xml into d33f2d88c2631d259a84e6575ae4a02324580621 on master.