Skip to content

[Config] Allow nonexistent file resources #20836

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 2 commits into from
Closed

[Config] Allow nonexistent file resources #20836

wants to merge 2 commits into from

Conversation

julienfalque
Copy link
Contributor

Q A
Branch? master
Bug fix? yes?
New feature? yes?
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Updates FileResource to also evaluate freshness against file existence.

Currently, FileResource assumes that the file always exists. I have a use case where this is not necessarily true: for tests purposes, I create a TestKernel class that uses MicroKernelTrait to define all the configs directly, except some values that are defined in phpunit.xml and phpunit.xml.dist files. Since the tests can pass with the default values from phpunit.xml.dist, creating the phpunit.xml file is not required. So in TestKernel I have to do this:

if (file_exists('phpunit.xml')) {
    $container->addResource(new FileResource('phpunit.xml'));
} else {
    $container->addResource(new FileExistenceResource('phpunit.xml'));
}

Maybe it should be a new class instead, WDYT?

@julienfalque julienfalque changed the title File resource [Config] Allow nonexistent file resources Dec 8, 2016
@nicolas-grekas nicolas-grekas added this to the 3.x milestone Dec 9, 2016
@xabbuh
Copy link
Member

xabbuh commented Dec 19, 2016

Not sure if we need this feature. But in this case I'd prefer a new parameter instead of a new resource type.

@julienfalque
Copy link
Contributor Author

I'm not sure about adding a parameter.

The current behavior is to check that the file still exists before evaluating freshness and return false if it doesn't. So it already detects when a file is removed, despite being more of a side-effect than something truly desired, I guess. A parameter would toggle the detection of the file creation, but would have no effect on the detection of its removal because we can't evaluate freshness against a missing file anyway.

Or maybe FileResource should throw an exception (or deprecation notice to keep BC) when the file is removed and said parameter does not allow it, but I'm not sure such strictness would make sense.

@sstok
Copy link
Contributor

sstok commented Dec 26, 2016

Maybe a Combined[File]Resource (or a better name) where you can add multiple [File]Resources and check until one gives false.

Combined[File]Resource([
    new FileExistenceResource('phpunit.xml'), 
    new FileResource('phpunit.xml')
]);

@julienfalque
Copy link
Contributor Author

That would still require changes to FileResource as it currently throws an exception if the file does not exist.

Furthermore, having such resource class to combine other resources would be superfluous: the container is recompiled as soon as at least one of its resources is updated, we can simply add both resources from your example to the container directly.

@nicolas-grekas
Copy link
Member

Closing in favor of #21408 which wraps the required logic and should be the way to go now.
Thanks for the PR.

@julienfalque julienfalque deleted the file-resource branch February 14, 2017 13:53
@nicolas-grekas nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug Feature Status: Needs Review
Projects
None yet
Milestone
3.3
Development

Successfully merging this pull request may close these issues.
5 participants
@julienfalque @xabbuh @sstok @nicolas-grekas @carsonbot