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.

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

Fluent mapping path loader #48

Open
wants to merge 1 commit into
base: 1.1
Choose a base branch
from
Open

Fluent mapping path loader #48

wants to merge 1 commit into from

Conversation

patrickbrouwers
Copy link
Contributor 

'paths'         => [
    base_path('src/AR1/Infrastructure/Mappings'),
    base_path('src/2/Infrastructure/Mappings'),
],

@patrickbrouwers patrickbrouwers requested a review from guiwoda July 4, 2017 20:10
@patrickbrouwers patrickbrouwers force-pushed the feature/paths-loader branch 2 times, most recently from e2e1441 to d3487ef Compare July 4, 2017 20:15
guiwoda
guiwoda suggested changes Jul 4, 2017
View reviewed changes
src/FluentDriver.php
$sourceFile = realpath($sourceFile);
}

require_once $sourceFile;
Copy link
Contributor

Choose a reason for hiding this comment

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

@patrickbrouwers I don't like this strategy, because it depends on those files not being previously loaded. I don't know what would happen with opcache, for example.
I think we talked about this in the previous PR.
I'd agree to merge as-is, but I wouldn't trust this strategy for my projects.

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 is actually copy pasted from the AnnotationsDriver.

src/FluentDriver.php

$declared = get_declared_classes();

foreach ($declared as $className) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@patrickbrouwers this list could be huge. We could use some convention to skip building a ReflectionClass for each loaded class in the system... Or maybe parse filename and class name and only reflect on basename matches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But that would require people to actually have a ...Mapping.php suffix.

Like previous comment, this is also the way the AnnotationsDriver does things. It's cached in production anyway.

@patrickbrouwers patrickbrouwers force-pushed the feature/paths-loader branch 2 times, most recently from f6c1573 to 955ea6e Compare July 4, 2017 20:23
@patrickbrouwers patrickbrouwers mentioned this pull request Jul 4, 2017
Closed
@guiwoda guiwoda mentioned this pull request Jul 4, 2017
Open
@guiwoda guiwoda mentioned this pull request Nov 19, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guiwoda guiwoda guiwoda requested changes

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

Successfully merging this pull request may close these issues.
2 participants
@patrickbrouwers @guiwoda