fixes #21, ContentType folder name also being used for plugins for Conte...

[espaan]

...nt.

I have tested the functionality in zk135b20 by installing, viewing/editing a page and creating a new page.

[ghost]

Are these supposed to be entity repositories? If so they should be under Entity/Respository/

[craigh]

IMO, these types of operations should be in entity/Repository/ but the way that @phaidon coded this is a bit unusual and is not done in the standard Doctrine\EntityRepository method.

So, IMO, yes they should be in Entity\Repository\ but they would need changes to the concept of the code also.

[espaan]

To be honest they seem to be, however they don't inherit from EntityRepository. I don't exactly get this in between class, but wanted to prevent a class with Content pluging naming.

2013/1/9 Drak notifications@github.com

Are these supposed to be entity repositories? If so they should be under Entity/

— Reply to this email directly or view it on GitHubhttps://github.com/zikula-modules/Pages/pull/23#issuecomment-12049075.

[espaan]

That is not in this PR, only the name change. So we can pull this one and extend or discard and make a new one. I don;t mind.

2013/1/9 Craig Heydenburg notifications@github.com

IMO, these types of operations should be in entity/Repository/ but the way that @phaidon https://github.com/phaidon coded this is a bit unusual and is not done in the standard Doctrine\EntityRepository method.

So, IMO, yes they should be in Entity\Repository\ but they would need changes to the concept of the code also.

— Reply to this email directly or view it on GitHubhttps://github.com/zikula-modules/Pages/pull/23#issuecomment-12049531.

[craigh]

+1

I really wish @phaidon would join the process though.

[phaidon]

I will check it.

[espaan]

Thanks !

[phaidon]

The idea behind this code was to have a very easy page object. With this object I wanted to to do things like $page->remove(); without thinking of doctrine (flush), manually access controls (getAccessLevel) and automatically creations (see __construct).

This is also with a doctrine entity possible, but imho not so comfortable. But if you dont want want this class layer then fell free to rewrite this.

[craigh]

I am not opposed to the class layer per se, but I am opposed to the original name/location (using the ContentType name). So in that regard, I think this PR should be merged (although the name change to Repository is probably not right - maybe Operation or Access ??? the classname should be relevant to its purpose.)

Doctrine's custom EntityRepositories are mainly geared towards housing special DB calls that are more complex than the base Repositories available methods (e.g. ->find(), ->findBy(), etc). So some of what you are doing in this object layer more appropriately belongs in a custom repository. Some of your object layer hides calls/functions that frankly belong in the controller. That is the purpose of the controller - access control, DB interaction, etc. To further obscure those things seems unnecessary.

So I understand what you have done and why you choose to do so, I'm just not sure it is a 'best practice'. Especially as we move towards Doctrine and Symfony standards - we will need to align with what they do more than our own personal preferences.

I'm not available to do this work at the moment (I would like to work on Scribite and Dizkus) but I think you should reconsider this.

@Guite any thoughts on the philosophy/design practice here? (you would need to look at the whole module/concept)

[espaan]

I think the class if fine, if somebody want to rewrite to another design than also fine :-) Shall I re-do the Pull with a new name like Access ?

DONE

[craigh]

pull it.