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.

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

Move composer dependency to dev #556

Merged
merged 1 commit into from
May 31, 2019
Merged

Move composer dependency to dev #556

merged 1 commit into from
May 31, 2019

Conversation

Landerstraeten
Copy link
Contributor

No description provided.

@Landerstraeten Landerstraeten added this to the grumpy-seventies milestone Oct 25, 2018
@Landerstraeten Landerstraeten requested a review from veewee October 25, 2018 08:41
@veewee
Copy link
Contributor

veewee commented Oct 26, 2018

HI @Landerstraeten,

This won't do:

It is possible to remove the composer/composer dependency if we rewrite the 3 items mentioned above. It can be done by just manually parsing the root composer.json file of the project instead of relying on the composer loader / factory.

@Landerstraeten Landerstraeten force-pushed the composer-dependency branch 2 times, most recently from f0588da to cd8f9ee Compare October 28, 2018 12:10
janvernieuwe
janvernieuwe reviewed Nov 30, 2018
View reviewed changes
src/Console/Application.php Outdated
$configuration = null;
$rootPackage = null;
}
$composerFileLocation = getcwd().DIRECTORY_SEPARATOR.'composer.json';
Copy link
Member

Choose a reason for hiding this comment

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

Is this path always guaranteed correct?
if it can't find the composer.json file, grumphp will crash on file_get_contents

Copy link
Contributor

Choose a reason for hiding this comment

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

No it is not always available. probably better to detect composer.json from current directory and from all parents until one is found?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a check in ComposerFile.php on line 21

Copy link
Contributor

Choose a reason for hiding this comment

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

In the old codebase, there was a catch which returns an empty composer info.
Maybe it's best to catch the error from ComposerFile line 21 and also return an empty Composer instance?
That way, the code keeps on working if there is no composer.json in the cwd (which probably will happen more then once... ;) )

@Landerstraeten Landerstraeten force-pushed the composer-dependency branch 2 times, most recently from 2f71095 to c2e2473 Compare November 30, 2018 13:25
veewee
veewee reviewed Dec 2, 2018
View reviewed changes
src/Util/ComposerFile.php Outdated
*/
public function getConfigDefaultPath()
{
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't you check and return following param?
$extra['grumphp']['config-default-path'];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ow yes. Clearly something I forgot todo.

src/Console/Application.php Outdated
$configuration = null;
$rootPackage = null;
}
$composerFileLocation = getcwd().DIRECTORY_SEPARATOR.'composer.json';
Copy link
Contributor

Choose a reason for hiding this comment

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

No it is not always available. probably better to detect composer.json from current directory and from all parents until one is found?

composer.json Outdated Show resolved Hide resolved
@Landerstraeten Landerstraeten changed the base branch from grumpy-seventies to master February 22, 2019 07:15
@Landerstraeten Landerstraeten force-pushed the composer-dependency branch 3 times, most recently from 9e84839 to e54ad42 Compare February 25, 2019 06:39
veewee
veewee reviewed Feb 27, 2019
View reviewed changes
src/Locator/ConfigurationFile.php Outdated

return $this->locateConfigFileWithDistSupport($composerDefaultPath);
return null === $composerDefaultPath
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a bit more readable if shorter:

return $this->locateConfigFileWithDistSupport($composerDefaultPath ?: $defaultPath);

src/Console/Application.php Outdated
$configuration = null;
$rootPackage = null;
}
$composerFileLocation = getcwd().DIRECTORY_SEPARATOR.'composer.json';
Copy link
Contributor

Choose a reason for hiding this comment

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

In the old codebase, there was a catch which returns an empty composer info.
Maybe it's best to catch the error from ComposerFile line 21 and also return an empty Composer instance?
That way, the code keeps on working if there is no composer.json in the cwd (which probably will happen more then once... ;) )

src/Console/Command/ConfigureCommand.php
}

return $config->get('bin-dir', Config::RELATIVE_PATHS);
return $this->composer()->getComposerFile()->getBinDir();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe also safer to force a relative path as in the old codebase? Not sure too what folder the old code is making the path relative

@Landerstraeten Landerstraeten modified the milestones: 0.15.1, 0.16.0 May 16, 2019
@veewee
Copy link
Contributor

veewee commented May 24, 2019

@Landerstraeten : Can you tell me what the state of this PR is? I'dd love to get this one merged in.
I'll be bumping the composer version in order to get #638 to work.
It seems better to use composer/composer:^1.8 as a dev dependency and check if our composer plugin also works with version composer/composer:^1.1 (which is the latest)

@Landerstraeten Landerstraeten merged commit c675400 into phpro:master May 31, 2019
@Landerstraeten Landerstraeten deleted the composer-dependency branch May 31, 2019 07:40
@jyggen jyggen mentioned this pull request Sep 9, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janvernieuwe janvernieuwe janvernieuwe left review comments

@veewee veewee veewee approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
0.16.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Landerstraeten @veewee @janvernieuwe