Skip to content

Switch source from tagged services to custom repository of entities#1

Open
Nerobrain wants to merge 5 commits intomakinacorpus:masterfrom
Nerobrain:master
Open

Switch source from tagged services to custom repository of entities#1
Nerobrain wants to merge 5 commits intomakinacorpus:masterfrom
Nerobrain:master

Conversation

@Nerobrain
Copy link
Copy Markdown

Sorry for my bad english. Just try to use and extend your bundle.

@pounard
Copy link
Copy Markdown
Member

pounard commented Jun 4, 2017

Wow, thanks a lot for this!

There's a lot of interesting changes in there, but the pull request is too big, I'd like to have multiple but smaller issues to review.

I'll try to take some time to comment, can't do right now, but in the next few days I'll do a complete review.

@pounard
Copy link
Copy Markdown
Member

pounard commented Jun 19, 2018

Ok, it's been a long time, first of all, I have lots of notes about your changes:

  • why did you change so many method names ? It's wrong because breaks backward compatibility, and it's unnecessary because it pollutes the diff with useless changes,
  • you hardcode Doctrine dependency within this bundle, which is something I do not want, I actually do maintain Symfony project without doctrine,
  • for a Doctrine implementation, you should place it in a src/Bridge/Doctrine folder, and build a meaningful configuration for users (using the Configuration component and some magic to autoregister sources for components the user enabled).

That said, there's probably some nice modifications within your changes, but you should fix the rest first.

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.

2 participants