Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI] FileLoaders: Allow to explicit type to load#20611
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ro0NL left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
| returnfalse; | ||
| } | ||
| if (null !==$type &&'ini' ===$type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would make these the final returns, ie.return 'ini' === $type;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Indeed it's better 👍 Thanks.
chalasr commentedNov 23, 2016
👍 |
| returntrue; | ||
| } | ||
| returnnull !==$type &&'ini' ===$type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
return 'ini' === $type; should be enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oops. Forgot to update after#20611 (comment)... Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Shouldn't we checktype first? If I set this option explicitly, I expect it to have top priority:
publicfunctionsupports($resource,$type =null){if (!is_string($resource)) {returnfalse; }return'ini' ===$type ||'ini' ===pathinfo($resource,PATHINFO_EXTENSION);}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@javiereguiluz the extension is checked only when the type is null. So there is no question of priority
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the type will have priority if explicitly provided with current implementation, as the above condition has anull === $type check.
The difference with your snippet is that the loader won't returntrue in case the type was explicitly provided, but notini and the extension isini (another loader should try)
javiereguiluzJan 10, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the loader won't return true in case the type was explicitly provided, but not ini and the extension is ini (another loader should try)
@ogizanagi in that case, this code will be: ...Maybe I'm missing something here.
updated: I see my mistake now. Thanks!
@stof maybe "priority" is not the best word, but the new behavior is:"do whatever "type" tells you ... if it doesn't tell you anything, fall back to the file extension".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What I meant is calling:
supports('services.ini, 'ini')should returntruesupports('services.foo, 'ini')should returntruesupports('services.ini, 'yaml')should returnfalse
The third case will be erroneous with your suggestion.
stof commentedNov 24, 2016
This is a new feature IMO, not a bugfix (we never said that |
xabbuh commentedNov 24, 2016
@stof The PR is already labelled as a feature and is based on the |
| returntrue; | ||
| } | ||
| returnnull !==$type &&'php' ===$type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
null !== $type should be removed in all loaders. It is useless as the condition'php' === $type already ensures is is not null
ogizanagiJan 10, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Don't know what happened here, I'm pretty sure I fixed this a long time ago... Thanks.
javiereguiluz commentedJan 10, 2017
👍 Status: reviewed |
fabpot commentedJan 10, 2017
Thank you@ogizanagi. |
…anagi)This PR was merged into the 3.3-dev branch.Discussion----------[DI] FileLoaders: Allow to explicit type to load| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#20308| License | MIT| Doc PR | Not yet(fabbot will scream out regarding the PR fixtures)Commits-------6b660c2 [DI] FileLoaders: Allow to explicit type to load
Uh oh!
There was an error while loading.Please reload this page.
(fabbot will scream out regarding the PR fixtures)