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

New Symfony 4 bin/composer directory structure #9

Merged
merged 5 commits into from
Jul 30, 2019
Merged

New Symfony 4 bin/composer directory structure #9

merged 5 commits into from
Jul 30, 2019

Conversation

ChrisKinnan
Copy link
Contributor

Hi thanks for this helpful bundle. The update script anticipates the need to use 'bin' rather than 'app' but the method useNewDirectoryStructure is looking for symfony-var-dir option. I added the symfony-bin-dir directory as an explicit option and the useNewDirectoryStructure method to look for that. I think this will help keep the script working with projects independent of 'symfony-var-dir' . Thanks!

Hi thanks for this helpful bundle. The update script anticipates the need to use 'bin' rather than 'app' but the method useNewDirectoryStructure is looking for symfony-var-dir option.  I added the symfony-bin-dir directory as an explicit option and the useNewDirectoryStructure method to look for that.  I think this will help keep the script working with projects independent of 'symfony-var-dir' .  Thanks!
@coveralls
Copy link

coveralls commented Jul 26, 2019

Coverage Status

Coverage remained the same at 25.899% when pulling 5dc008b on ChrisKinnan:patch-1 into bb3cd97 on gpslab:master.

@peter-gribanov
Copy link
Member

Thanks for report

@@ -151,7 +152,7 @@ private static function hasDirectory(Event $event, $config_name, $path, $action_
*/
private static function useNewDirectoryStructure(array $options)
{
return isset($options['symfony-var-dir']) && is_dir($options['symfony-var-dir']);
return isset($options['symfony-bin-dir']) && is_dir($options['symfony-bin-dir']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change may break BC. Need check symfony-var-dir and symfony-bin-dir options.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure what symfony-var-dir is doing...it looks like it is just acting as a signal for a certain version of Symfony? Are users setting that value outside of the script? Thanks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry. I hurried. This method is needed to identify the new folder structure in Symfony.
This code is a copy/paste of the SensioDistributionBundle.
https://github.com/sensiolabs/SensioDistributionBundle/blob/master/Composer/ScriptHandler.php
Are you sure that this code does not work in Symfony 4 as it is?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it breaks-- Symfony put console into the bin/console directory

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can solve the problem by setting up composer.json:

{
   # ...
   "extra": {
        "symfony-var-dir": "var",
        "symfony-bin-dir": "bin"
    }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve replacement option symfony-var-dir to symfony-bin-dir 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may not want to make any change to useNewDirectoryStructure method, this code isn't needed for SF 4 and might break someone's older implementation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can offer this option which, in theory, should work the same in Symfony 4 and older versions.

if (isset($options['symfony-bin-dir'])) {
    return is_dir($options['symfony-bin-dir']);
} else {
    return is_dir('bin');
}

Although it can also break BC.

src/Composer/ScriptHandler.php Outdated Show resolved Hide resolved
Took a deeper dive on this and it turns out we can just call the Command directly.  Updated the docs, thank you.
@peter-gribanov peter-gribanov merged commit bd4e571 into gpslab:master Jul 30, 2019
@peter-gribanov
Copy link
Member

Thanks for the work you've done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants