add doctrine orm mapper and cleanup the base mapper to have no phpcr specific code #55
Conversation
| { | ||
| $idstring = parent::createSubject($object); | ||
|
|
||
| return $this->canonicalName(get_class($object)) . "-$idstring"; |
There was a problem hiding this comment.
this will expose the FQN in the frontend. do we want this? is there something to ask doctrine class metadata for a short name that we can afterwards use to get the repository / original class?
/cc @lsmith77
There was a problem hiding this comment.
In perfect scenario we can configure subject per class. What do you think? Can we do this in createphp metadata?
There was a problem hiding this comment.
@lsmith77 well, getting the metadata is either the FQCN or alias:relative_name (alias: gets replaced by the namespace corresponding to the alias). But it is still leaking the class name to the frontend.
If you don't want to leak class names, you should build a map of class names to frontend resource (and back if necessary). such mapping could then be useful for other implementations
There was a problem hiding this comment.
Will check if we can put that into the metadata somewhere, optionally and otherwise fallback to the class. Could the backslash cause issues? Should i use . instead?
There was a problem hiding this comment.
i created #57 for this. i think the framwork would be ready for that, its just that nobody implemented the additional step needed to build a map.
|
the bundle will need something to provide the orm mapper. i added an issue at CreateBundle symfony-cmf/create-bundle#87 - @pjedrzejewski if you want to play with this already, you could use the fork of createphp with the branch and try the bundle thing. might give us valuable inputs as well. thanks for all the feedback. i will clean up the PR as soon as possible. these are things that won't make it into cmf 1.0.0 we are releasing today. i have to focus on one after the other. |
| if (count($ids) < 2) { | ||
| throw new RuntimeException("Invalid subject: $subject"); | ||
| } | ||
| $repository = $this->om->getRepository($ids[0]); |
There was a problem hiding this comment.
we discovered that accidentally having / at the beginning of a FQN will lead to php finding the class and loading it, but not realizing its already loaded so it tries to load again and does a "FatalErrorException: Compile Error: Cannot redeclare class ...".
we hacked around this with str_replace('/', '', ltrim($ids[0], '/'));
|
i now added a test about the escaping as well, seems to work fine. @flack could you try to activate travis for PRs in this repository? would be nice to see if travis agrees with my local setup that this is working. |
|
@dbu This should work already, or at least it did last year. travis seems to be down ATM, so maybe this is the reason why you don't get any feedback. When it's back up, I'll take a look to see if the settings are ok |
|
oh, indeed. i was wondering why i don't see any travis info in the PR header. anyways, tests locally succeed. does it make sense for you what we do here? ok to merge, or do you want the BC method? should we start a CHANGELOG.md file to record BC breaks and other notable changes? |
|
+1 for CHANGELOG.md, it's getting quite hard to remember all the relevant changes between releases. I think the In general the PR looks good to me. The ORM subject logic is not exactly pretty, but I guess it can't be helped. I've added a couple of line notes about minor concerns, but otherwise, this is good to merge |
|
okay, updated to escape |
add doctrine orm mapper and cleanup the base mapper to have no phpcr specific code
the changes in the phpcr mapper should be no bc break so it should be fine to put into the 0.9 which master currently represents.
this PR does not cover to not expose class names publicly as this involves more refactoring on the library. i created a feature request for that: #57 . same holds about #58, it would be nice to be able to mix objects from various mappers but its out of scope for this PR.