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

[WIP] added support for PHPCR #340

Merged
merged 1 commit into from
Dec 4, 2013
Merged

Conversation

lsmith77
Copy link
Contributor

this also improves the detection for when the ORM is actually enabled.

requires schmittjoh/serializer#184
adding MongoDB should be easy too

$previousId = $id;
}
}
$registries = array();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the duplicate code here should be refactored

@lsmith77
Copy link
Contributor Author

I have added a compiler pass to more reliably determine if ORM/PHPCR ODM is really available since both DoctrineBundle and DoctrinePHPCRBundle can be used without having the ORM/PHPCR ODM available

return;
}

$registries = array(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this entire code here looks ugly imho but I didn't come up with a nicer solution. then again I wonder if it would not be more readable by simply duplicating the code

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It often helps to split things up into smaller methods.

Is the $managerId below necessary? Why not check for the registry directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i did a few tweaks. code is now easier to read

@lsmith77
Copy link
Contributor Author

@schmittjoh ok to merge?

@schmittjoh
Copy link
Owner

Will try to find some time this evening to review/merge some of the PRs
here.

On Thu, Nov 28, 2013 at 2:06 PM, Lukas Kahwe Smith <notifications@github.com

wrote:

@schmittjoh https://github.com/schmittjoh ok to merge?


Reply to this email directly or view it on GitHubhttps://github.com//pull/340#issuecomment-29462246
.

@awdng
Copy link

awdng commented Dec 3, 2013

+1 would love to use this, currently doing manual encoding on PHPCR Documents.

schmittjoh added a commit that referenced this pull request Dec 4, 2013
[WIP] added support for PHPCR
@schmittjoh schmittjoh merged commit 9cdfef0 into schmittjoh:master Dec 4, 2013
@lsmith77 lsmith77 deleted the phpcr branch December 4, 2013 17:13
@lsmith77
Copy link
Contributor Author

lsmith77 commented Dec 4, 2013

thx .. now its been 4 months since the last release of this bundle and seralizer. speaking of which .. any plans for 1.0? :)

@schmittjoh
Copy link
Owner

Yeah, but we should probably make the license switch before 1.0.

@lsmith77
Copy link
Contributor Author

lsmith77 commented Dec 5, 2013

lets do it http://licenses.beberlei.de :)

@schmittjoh
Copy link
Owner

Yep, it's on my TODO list :)

@lsmith77
Copy link
Contributor Author

alright .. but if is going to take longer then it might be good to do a new 0.x release of the bundle and the lib ..

@schmittjoh
Copy link
Owner

I don't hope so, but just so that you don't need to rely on dev versions, I've tagged new releases for both packages (bundle & library).

@lsmith77
Copy link
Contributor Author

thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants