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.

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

Allow user to set the project dir #30651

Merged
merged 1 commit into from Mar 23, 2019

Conversation

Projects
None yet
8 participants
@tdutrion
Copy link
Contributor

commented Mar 22, 2019

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

Currently, the project directory is defined by the location of the composer.json file.

That file is not required in production, which therefore breaks the method getProjectDir (who sends back null).
The offered solution, while working, requires the developer to implement it, and uses inheritance override, while a more aesthetic solution could be used.

This does not fix the behaviour, but allows the developer to pass the project dir as a parameter.

While this solution does not include BC break or anything, it is important to notice that it includes
an optional parameter.

Object instantiation in the framework bundle recipe could be updated as follow (in another PR):

$kernel = new Kernel($_SERVER['APP_ENV'], (bool) $_SERVER['APP_DEBUG']);
$kernel = new Kernel($_SERVER['APP_ENV'], (bool) $_SERVER['APP_DEBUG'], dirname(__DIR__));
Allow user to set the project dir
Currently, the project directory is defined by the location of the composer.json file.
That file is not required in production, which therefore breaks the method getProjectDir (who sends back null).
This does not fix the behaviour, but allows the developer to pass the project dir as a parameter.

@chalasr chalasr added the HttpKernel label Mar 23, 2019

@chalasr chalasr added this to the next milestone Mar 23, 2019

@fabpot

fabpot approved these changes Mar 23, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

Thank you @tdutrion.

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

@tdutrion Can you submit a PR for the recipe?

@fabpot fabpot merged commit c40017d into symfony:master Mar 23, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 23, 2019

feature #30651 Allow user to set the project dir (tdutrion)
This PR was merged into the 4.3-dev branch.

Discussion
----------

Allow user to set the project dir

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |  <!-- symfony/symfony-docs#... required for new features -->

Currently, the project directory is defined by the location of the composer.json file.

That file is not required in production, which therefore [breaks the method getProjectDir](#23950) (who sends back null).
The offered solution, while working, requires the developer to implement it, and uses inheritance override, while a more aesthetic solution could be used.

This does not fix the behaviour, but allows the developer to pass the project dir as a parameter.

While this solution does not include BC break or anything, it is important to notice that it includes
**an optional parameter**.

[Object instantiation in the framework bundle recipe](https://github.com/symfony/recipes/blob/master/symfony/framework-bundle/4.2/public/index.php#L23) could be updated as follow (in another PR):

```php
$kernel = new Kernel($_SERVER['APP_ENV'], (bool) $_SERVER['APP_DEBUG']);
```

```php
$kernel = new Kernel($_SERVER['APP_ENV'], (bool) $_SERVER['APP_DEBUG'], dirname(__DIR__));
```

Commits
-------

c40017d Allow user to set the project dir
@tdutrion

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2019

@tdutrion Can you submit a PR for the recipe?

Done in symfony/recipes#555

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

I'm working on the changes needed for this in the docs ... and I wondered why do we need all this logic to calculate the project dir:

public function getProjectDir()
{
if (null === $this->projectDir) {
$r = new \ReflectionObject($this);
$dir = $rootDir = \dirname($r->getFileName());
while (!file_exists($dir.'/composer.json')) {
if ($dir === \dirname($dir)) {
return $this->projectDir = $rootDir;
}
$dir = \dirname($dir);
}
$this->projectDir = $dir;
}
return $this->projectDir;
}

Do you consider making the string $projectDir argument mandatory in the constructor so we can remove this? Or is this logic still needed for some reason? Thanks.

@tdutrion

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

While I'd love to have it mandatory, it seems to me that it would be a breaking change, so that would need to wait for a major version (let me know if I'm wrong there).

@stof

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

Well, the drawback of relying on a constructor argument is that it means that each place instantiating the kernel has to know which value to pass it.
Updating your front controller and your console already means updating 2 places (for which not passing the same absolute path would cause really weird things to happen). But there are other places instantiating it:

  • tests extending KernelTestCase or one of its child classes
  • Behat's Symfony extension also instantiate the kernel
  • maybe other places in other ecosystems

Requiring all these places to provide a configuration setting for which you need to pass the same project root looks like a bad experience to me.
That's precisely the reason why we introduced getProjectDir with a sensible implementation for most projects (the only projects breaking it are the one where you delete the composer.json before deployment), documenting that the supported extension point for people needing a different logic there is to define the method in their Kernel.

To me, a constructor argument here is actually a much worse DX than overriding the method.

@stof

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

What we could do though is to throw an exception when the composer.json could not be found (asking to override the method or to keep the file) instead of returning a broken root dir, to improve DX.

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

throw an exception when the composer.json could not be found (asking to override the method or to keep the file)

This is nice ... because it solves the problem without forcing the user to look for or read any docs. Just read the great exception message, make the changes, and you're done!

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

Reading the discussion here, I'm wondering if you should revert this change.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

Makes sense. Let's revert.

@tdutrion

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

Don't forget symfony/recipes#555 if you revert

@jvasseur

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

One solution (the one I use in my projects) is instead to override the getProjectDir method in src/Kernel.php to return dirname(__DIR__). Maybe this could be added to the recipe ?

@tdutrion

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

While this solution work, it's not as decent from a software engineering point of view, as it's again hiding the setting somewhere rather than forcing the developer who calls it to think where it's root his.

Works for most symfony standard setup though, could be tricky if people do change the setup without understanding fully how things work.

@jvasseur

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

While this solution work, it's not as decent from a software engineering point of view, as it's again hiding the setting somewhere rather than forcing the developer who calls it to think where it's root his.

Well it feels more logical to me to put it in the Kernel.php file since it's the file responsible for managing your project structure (cache, logs, config).

Works for most symfony standard setup though, could be tricky if people do change the setup without understanding fully how things work.

It's true if you define it in the index.php or console file to.

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

reverted

fabpot added a commit that referenced this pull request Mar 29, 2019

Revert "feature #30651 Allow user to set the project dir (tdutrion)"
This reverts commit aa12dd0, reversing
changes made to 7d01aae.

alquerci added a commit to alquerci/symfony that referenced this pull request Mar 31, 2019

alquerci added a commit to alquerci/symfony that referenced this pull request Apr 10, 2019

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.