Skip to content

Resolve custom repository classes #47

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

Resolve custom repository classes #47

wants to merge 4 commits into from

Conversation

mcfedr
Copy link
Contributor

@mcfedr mcfedr commented Feb 12, 2019 •
edited
Loading

I've taken #18 and fixed it up and rebased on master

@mcfedr mcfedr force-pushed the mapping-poc branch 3 times, most recently from 64e1ecc to 392eefe Compare February 12, 2019 10:49
mcfedr
mcfedr commented Feb 12, 2019
View reviewed changes
composer.json
"doctrine/annotations": "^1.5",
"doctrine/persistence": "^1.1",
"doctrine/dbal": "^2.5",
"doctrine/event-manager": "^1.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm quite unconvinced of the need for this - composer-require-checker demands this, but actually if you dont configure 'mapping' you wont need any of this, and if you do, you should have it installed anyway

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why any of the changes in composer.json were needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its because of the code that loads/creates the entity manager, but if we use the approach in #40 this wont be needed.

@mcfedr
Copy link
Contributor Author

mcfedr commented Feb 12, 2019

Will make sense to rebase this on top of #46 because the code moves into the abstract class

@ondrejmirtes
Copy link
Member

Hi, the work done by @lcobucci in #40 seems like a more solid base to work on.

I really don't like how src/DoctrineClassMetadataProvider.php constructor looks like - it violates the principle "don't do any work in constructor", for example people without the pdo_sqlite extension would be out of luck.

@ondrejmirtes
Copy link
Member

Unfortunately there's some unresolved discussion in #39 and #40 is based on that.

@mcfedr mcfedr mentioned this pull request Feb 12, 2019
Closed
@mcfedr
Copy link
Contributor Author

mcfedr commented Feb 12, 2019

Makes sense, #48 fixes the issues in #39.

If you merge that ill see about fixing up #40 as well.

@mcfedr mcfedr closed this Feb 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ondrejmirtes ondrejmirtes ondrejmirtes left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mcfedr @ondrejmirtes @malarzm