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

craue · 2015-04-16T09:16:08Z

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 · 2015-04-16T09:18:46Z

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 · 2015-04-16T11:22:24Z

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

Only as a dev requirement.

craue · 2015-04-21T08:38:19Z

@rvanlaak: Fine to merge?

rvanlaak · 2015-04-21T09:00:00Z

Will check the PR branch later today

rvanlaak · 2015-04-22T09:18:25Z

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

Will try to override the XML mapping now.

rvanlaak · 2015-04-22T09:22:57Z

Doctrine tries to drop the UNIQUE index:

DROP INDEX UNIQ_B95BA9425E237E06 ON craue_config_setting;

rvanlaak · 2015-04-22T09:27:18Z

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 · 2015-04-22T11:01:18Z

...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. 😏

Doctrine tries to drop the UNIQUE index

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

rvanlaak · 2015-04-22T15:46:47Z

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! 👍

craue · 2015-04-23T13:51:46Z

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 · 2015-04-23T13:54:23Z

Will pull the changes and take a look into that within the hour 👍

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

rvanlaak · 2015-04-23T14:48:45Z

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

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

craue · 2015-04-23T14:53:24Z

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 · 2015-04-23T14:54:55Z

Yeah, the following:

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

craue · 2015-04-23T15:03:58Z

And you want to change the type of the value field, right? 😏
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 · 2015-04-23T15:11:09Z

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

rvanlaak · 2015-04-28T12:47:33Z

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 · 2015-05-04T09:54:30Z

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 · 2015-06-01T18:49:06Z

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

craue · 2017-04-04T13:37:50Z

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

rvanlaak · 2017-04-04T13:49:13Z

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 · 2017-04-04T14:23:26Z

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 · 2017-04-04T15:05:06Z

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

craue · 2017-04-12T11:32:08Z

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

rvanlaak

Yes, it is ready for merge. Did place some minor comments, mainly related to removing year numbers ;-)

rvanlaak · 2017-04-12T11:55:42Z

Entity/SettingInterface.php

+
+/**
+ * @author Christian Raue <christian.raue@gmail.com>
+ * @copyright 2011-2017 Christian Raue


I'd advise you to remove the years, to prevent you to have a task on January 1st.

I prefer to keep the year for now. It's not a big deal to perform a search and replace once a year. 😆

rvanlaak · 2017-04-12T11:56:01Z

DependencyInjection/Configuration.php

+						->thenInvalid('The driver "%s" is not supported. Please choose one of ' . json_encode($supportedDrivers))
+					->end()
+				->end()
+				// when at least two values are defined (Symfony < 3.1 throws an exception with only one value)


Commented code can be removed?

Done, added a TODO task instead.

rvanlaak · 2017-04-12T11:57:25Z

Resources/config/doctrine-mapping/BaseSetting.orm.xml

+<?xml version="1.0" encoding="UTF-8" ?>
+<!--
+	Author: Christian Raue <christian.raue@gmail.com>
+	Copyright: 2011-2017 Christian Raue


Same thing about the year here ;)

rvanlaak · 2017-04-12T12:13:08Z

composer.json

@@ -16,17 +16,17 @@
 		}
 	],
 	"require": {
-		"php": "^5.3.2|~7.0",
-		"symfony/framework-bundle": "~2.3|~3.0"
+		"php": "^5.3.9|~7.0",


Will this PR release 2.0 of this bundle? If so, what about bumping it to >=5.5.9?

I guess 2.0 can be released soon, yes. But I'd like to keep Symfony 2.7 compatibility.

…ng it

coveralls · 2017-04-13T08:09:37Z

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

craue force-pushed the doctrine-mapping-xml branch 2 times, most recently from 13b181d to 50e07a1 Compare April 20, 2015 09:36

craue added the improvement label Apr 20, 2015

craue force-pushed the doctrine-mapping-xml branch from 50e07a1 to 04f6b27 Compare April 21, 2015 09:20

craue force-pushed the doctrine-mapping-xml branch from 31dfe46 to 5a39944 Compare April 23, 2015 13:47

rvanlaak mentioned this pull request May 26, 2016

Reduce column size to fit utf8mb4 index rvanlaak/SettingsBundle#87

Open

craue force-pushed the doctrine-mapping-xml branch 2 times, most recently from 96b6ca1 to b5f8872 Compare April 4, 2017 13:23

craue force-pushed the doctrine-mapping-xml branch from b5f8872 to 0184725 Compare April 4, 2017 14:11

craue force-pushed the doctrine-mapping-xml branch from 0184725 to 4efac97 Compare April 4, 2017 14:23

craue added BC break new feature and removed improvement labels Apr 4, 2017

rvanlaak reviewed Apr 12, 2017

View reviewed changes

craue added 2 commits April 13, 2017 09:23

use XML instead of annotations for Doctrine mapping to allow overridi…

87ea00c

…ng it

bumped Symfony dependency to 2.7, removed legacy code

3f447e1

craue force-pushed the doctrine-mapping-xml branch from 4efac97 to 3f447e1 Compare April 13, 2017 07:27

craue merged commit e661a70 into master Apr 13, 2017

craue deleted the doctrine-mapping-xml branch April 13, 2017 08:25

This was referenced Apr 13, 2017

Add MongoDB support #8

Open

Enhanced the value field to store bigger strings than 255 chars #5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

craue commented Apr 16, 2015

rvanlaak commented Apr 16, 2015

craue commented Apr 16, 2015

craue commented Apr 21, 2015

rvanlaak commented Apr 21, 2015

rvanlaak commented Apr 22, 2015

rvanlaak commented Apr 22, 2015

rvanlaak commented Apr 22, 2015

craue commented Apr 22, 2015

rvanlaak commented Apr 22, 2015

craue commented Apr 23, 2015

rvanlaak commented Apr 23, 2015

rvanlaak commented Apr 23, 2015

craue commented Apr 23, 2015

rvanlaak commented Apr 23, 2015

craue commented Apr 23, 2015

rvanlaak commented Apr 23, 2015

rvanlaak commented Apr 28, 2015

craue commented May 4, 2015

craue commented Jun 1, 2015

craue commented Apr 4, 2017

rvanlaak commented Apr 4, 2017

craue commented Apr 4, 2017

coveralls commented Apr 4, 2017 •

edited

craue commented Apr 12, 2017

rvanlaak left a comment

rvanlaak Apr 12, 2017

craue Apr 13, 2017

rvanlaak Apr 12, 2017

craue Apr 13, 2017

rvanlaak Apr 12, 2017

rvanlaak Apr 12, 2017

craue Apr 13, 2017

coveralls commented Apr 13, 2017 •

edited

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

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

Conversation

craue commented Apr 16, 2015

rvanlaak commented Apr 16, 2015

craue commented Apr 16, 2015

craue commented Apr 21, 2015

rvanlaak commented Apr 21, 2015

rvanlaak commented Apr 22, 2015

rvanlaak commented Apr 22, 2015

rvanlaak commented Apr 22, 2015

craue commented Apr 22, 2015

rvanlaak commented Apr 22, 2015

craue commented Apr 23, 2015

rvanlaak commented Apr 23, 2015

rvanlaak commented Apr 23, 2015

craue commented Apr 23, 2015

rvanlaak commented Apr 23, 2015

craue commented Apr 23, 2015

rvanlaak commented Apr 23, 2015

rvanlaak commented Apr 28, 2015

craue commented May 4, 2015

craue commented Jun 1, 2015

craue commented Apr 4, 2017

rvanlaak commented Apr 4, 2017

craue commented Apr 4, 2017

coveralls commented Apr 4, 2017 • edited

craue commented Apr 12, 2017

rvanlaak left a comment

Choose a reason for hiding this comment

rvanlaak Apr 12, 2017

Choose a reason for hiding this comment

craue Apr 13, 2017

Choose a reason for hiding this comment

rvanlaak Apr 12, 2017

Choose a reason for hiding this comment

craue Apr 13, 2017

Choose a reason for hiding this comment

rvanlaak Apr 12, 2017

Choose a reason for hiding this comment

rvanlaak Apr 12, 2017

Choose a reason for hiding this comment

craue Apr 13, 2017

Choose a reason for hiding this comment

coveralls commented Apr 13, 2017 • edited

coveralls commented Apr 4, 2017 •

edited

coveralls commented Apr 13, 2017 •

edited