Skip to content

Fluent mapping path loader #48

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
Open
Parent: Add a way to specify a naming strategy
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

if (!preg_match('(^phar:)i', $sourceFile)) {
$sourceFile = realpath($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
$includedFiles[] = $sourceFile;
}

$declared = get_declared_classes();
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

1 more reviewer

@guiwoda guiwoda guiwoda requested changes

Reviewers whose approvals may not affect merge requirements

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