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

[2.0][RFC] Add get to default repository #1613

Closed
wants to merge 22 commits into from

Conversation

malarzm
Copy link
Member

@malarzm malarzm commented Jun 23, 2017

Today I have realized that most of my custom repositories are beginning with get method which looks just like the one I have here. So how about adding it to core? If we say "yes" then here's still few things to do like:

  • change ObjectRepository occurrences to new Repository
  • especially after rebasing odm-ng atop current master
  • move PHPDocs to interface

Also since it's for 2.0 we may consider moving Repository-related classes around as we got ourselves "Repository" namespace just before 1.0, but that will mean more things to fix for users when upgrading.

@malarzm malarzm added this to the 2.0.0 milestone Jun 23, 2017
@alcaeus
Copy link
Member

alcaeus commented Jun 26, 2017

As mentioned in IRC, I see a few issues with the method:

  • We have find, findBy, findOneBy. Especially with find, it's not easy to understand the difference between find and get since they are not self-explanatory.
  • One could argue that the current API covers the use cases. For those cases where the code can't continue if no object is returned, the code may as well throw the exception themselves and throw a specific exception instead of a generic DocumentNotFoundException.
  • Last but not least, there's no comparable method in ORM and I don't know of any plans to add one. I'd always try to keep the APIs as comparable as possible, especially in those classes that don't deal with low-level logic that might depend on the database system in use.

Considering this, I'm 👎 on adding this.

@alcaeus alcaeus force-pushed the odm-ng branch 2 times, most recently from f07e68d to fa8edc9 Compare November 3, 2017 06:00
@malarzm malarzm closed this Dec 14, 2017
@malarzm malarzm deleted the add-get-to-repository branch December 14, 2017 14:20
@alcaeus alcaeus removed this from the 2.0.0 milestone Sep 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants