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] Mapped superclass #27

Merged
merged 9 commits into from Aug 5, 2014
Merged

[WIP] Mapped superclass #27

merged 9 commits into from Aug 5, 2014

Conversation

xavismeh
Copy link
Contributor

Refactored classes to allow usage out of the box or ease ability to override classes used

<?php

/*
* This file is part of the FOSUserBundle package.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this header file should be the EkinoWordpressBundle header file because "please view the LICENSE file that was distributed with this source code" is regarding this bundle

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 part of the code comes from FOSUserBundle so I was thinking it was nice to keep track of original authorship

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright, we can keep it

Copy link
Contributor

Choose a reason for hiding this comment

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

The header need to be the one from this project. However the copyright holder can be kept in the class comment. // cc @dbu

Copy link

Choose a reason for hiding this comment

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

the file is definitely not part of FOSUserBundle when its added in this bundle :-) according to MIT license you are free to copy it and add the header for this bundle.

i don't mind about the @author thing, do as you please.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, the header must be the EkinoWordpressBundle one but I think this is normal to keep your @author tag, thank you for that clarification.

@eko
Copy link
Collaborator

eko commented Jul 30, 2014

@xavismeh,

I think this is a great to allow extending models.

However, can you explain me why there are 2 Doctrine configuration files? (for entities and for models): Resources/config/doctrine/*.orm.xml and Resources/config/doctrine/models/*.orm.xml?

Thank you!

@xavismeh
Copy link
Contributor Author

¡ola!
The split has been made this way to allow 2 behaviors :
1 - use the bundle out of the box, with least configuration as possible => "standard" users that do not want to be bothered by configuration and just want it working
2 - allow override of anything related to entities (entities themselves, repos, ...) => advanced users that want to implements a more complex logic

To be honest, excluding repositories override, I don't have a clue yet why it should be overridden but I didn't want to restrict developers' ability to extend anything

@eko
Copy link
Collaborator

eko commented Jul 31, 2014

Alright @xavismeh, so let's keep it like that.

Can you just add some little lines in the README.md file to explain developers that they optionally can override entities & repos (and how they should proceed)?

Thank you :)

@rande
Copy link
Contributor

rande commented Jul 31, 2014

Can you add test for RegisterMappingsPass ? thanks

* register mappings compiler pass yet.
*
* @deprecated Compatibility class to make the bundle work with Symfony < 2.3.
* To be removed when this bundle drops support for Symfony < 2.3
Copy link

Choose a reason for hiding this comment

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

btw, do you still support symfony < 2.3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good remark : it should be compliant with 2.1+ but not tested (yet). Will add this environment cases in travis

@rande
Copy link
Contributor

rande commented Aug 1, 2014

@xavismeh you need to rebase this PR

eko added a commit that referenced this pull request Aug 5, 2014
@eko eko merged commit 6c514cb into ekino:master Aug 5, 2014
@eko
Copy link
Collaborator

eko commented Aug 5, 2014

Thank you @xavismeh

@xavismeh xavismeh deleted the mapped-superclass branch August 5, 2014 12:57
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

4 participants