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

Fix restoring working directory when building legacy CLI kernel handler #33

Merged
merged 2 commits into from Aug 31, 2015

Conversation

emodric
Copy link
Collaborator

@emodric emodric commented Aug 28, 2015

After merging #31, and associated PR in ezsystems/ezpublish-kernel, I get an error when running assets:install command:

eddie@abyss: /var/www/html/netgensite $ php ezpublish/console assets:install --symlink --relative -v

  [InvalidArgumentException]                  
  The target directory "web" does not exist.  

Exception trace:
 () at /var/www/html/netgensite/vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Command/AssetsInstallCommand.php:74
 Symfony\Bundle\FrameworkBundle\Command\AssetsInstallCommand->execute() at /var/www/html/netgensite/vendor/symfony/symfony/src/Symfony/Component/Console/Command/Command.php:259
 Symfony\Component\Console\Command\Command->run() at /var/www/html/netgensite/vendor/symfony/symfony/src/Symfony/Component/Console/Application.php:886
 Symfony\Component\Console\Application->doRunCommand() at /var/www/html/netgensite/vendor/symfony/symfony/src/Symfony/Component/Console/Application.php:195
 Symfony\Component\Console\Application->doRun() at /var/www/html/netgensite/vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Console/Application.php:96
 Symfony\Bundle\FrameworkBundle\Console\Application->doRun() at /var/www/html/netgensite/vendor/symfony/symfony/src/Symfony/Component/Console/Application.php:126
 Symfony\Component\Console\Application->run() at /var/www/html/netgensite/ezpublish/console:27


assets:install [--symlink] [--relative] [--] [<target>]

This fixes the issue by making sure that the previously used working directory is restored after building CLI kernel handler, instead of restoring to web root (web/).

@andrerom
Copy link
Contributor

This seems to be incomplete. webrootDir is still used several places.

@emodric
Copy link
Collaborator Author

emodric commented Aug 28, 2015

It is, but I think that is intended, right?

@wizhippo
Copy link
Contributor

Has this always been the case? What does #31 do that changes this behavior? Why does the legacy kernel loader not restore the working director in the first place?

@emodric
Copy link
Collaborator Author

emodric commented Aug 28, 2015

I can't answer that unfortunately. Maybe your fix just revealed this issue now.

@wizhippo
Copy link
Contributor

@andrerom Do you mean in just the loader? For example the same fix should be applied to building the web legacy kernel too. webrootDir seems to only be need as a fallback for Kernel::leaveLegacyRootDir, which I'm not too sure about. When would Kernel not have a previous previousRunningDir to switch back to as once he kernel is constructed it would be set by the calling of enterLegacyRootDir?

@emodric
Copy link
Collaborator Author

emodric commented Aug 28, 2015

I think that restoring to webroot when building web handler is intended, because, after all, working directory for the web itself IS webroot and not project root.

But ofcourse, I might be wrong.

Maybe @lolautruche can give some insight

@emodric
Copy link
Collaborator Author

emodric commented Aug 28, 2015

@andrerom @wizhippo Or maybe he meant that it was still transfered to closure but not actually used. That was an oversight.

@andrerom
Copy link
Contributor

I think it looks ok now, as you said in web and rest context web folder is expected when not in legacy folder.

+1

@lolautruche
Copy link
Contributor

+1

andrerom added a commit that referenced this pull request Aug 31, 2015
Fix restoring working directory when building legacy CLI kernel handler
@andrerom andrerom merged commit 0852275 into ezsystems:master Aug 31, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants