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

[Enhancement] Consider removing all sylius_ database tables prefix #61

Closed
marcospassos opened this issue May 10, 2013 · 10 comments
Closed
Labels
Feature New feature proposals.

Comments

@marcospassos
Copy link
Contributor

Once there is no way (at least a simple way) to override the DB's tables/repositories names, I think the sylius_ prefix should be removed. If a prefix is desirable, it can be solved using a listener that can be disabled.

@winzou
Copy link
Contributor

winzou commented May 10, 2013

👎 prefixes keep sylius logic separated from the main app logic (in the db I mean).

@marcospassos
Copy link
Contributor Author

I think it should be an option, not a requirement. As I said, it's can be done through Doctrine's events, giving us both options. In the current way you have just one option.

@winzou
Copy link
Contributor

winzou commented May 10, 2013

Do you think about http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/cookbook/sql-table-prefixes.html ?
If so, is there a mean to prefix only a bunch of tables (I think of only the sylius ones)? It would be a good solution.

@marcospassos
Copy link
Contributor Author

Exactly. We can restrict the prefix only to Sylius tables, through namespace. So we can add a config option to allow prefix customization. It's a flexible and customizable solution, that allows enable/disable when desirable.

sylius:
    table_prefix: sylius_

@winzou
Copy link
Contributor

winzou commented May 10, 2013

👍 :)

@pjedrzejewski
Copy link
Member

I do not see big harm in prefixes, but if we can make them optional - +1, if we make them configurable - +2!

@pjedrzejewski
Copy link
Member

But I don't think we should remove them by default, I find it good practice, I clearly see what is part of custom app, and what is coming from Sylius, just like @winzou said!

@marcospassos
Copy link
Contributor Author

Yes, this is my intention.

@smalot
Copy link

smalot commented May 19, 2013

👍

pjedrzejewski pushed a commit that referenced this issue Aug 26, 2013
pjedrzejewski pushed a commit that referenced this issue Aug 29, 2013
@elliot
Copy link
Contributor

elliot commented May 9, 2014

Closable by #447 ?

CoderMaggie pushed a commit to CoderMaggie/Sylius that referenced this issue Jun 1, 2016
[CJMAX-55][CJMAX-56] Overriden account's address pages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature proposals.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants