NEW Use composer 2 #62

emteknetnz · 2022-04-20T00:07:36Z

Issue silverstripe/silverstripe-framework#10250

Switch from composer 1 to composer 2, also add the ?? operator to a few native function calls to ensure null values aren't passed in for PHP 8.1 compatibility

Create 3 + 3.0 branches and tag 3.0.0 when merged

Required for silverstripe/recipe-reporting-tools#26

emteknetnz · 2022-04-20T03:30:50Z

@spekulatius are you OK with us tagging a new major (v3) to support composer v2? It's required for PHP 8.1 support.

GuySartorelli · 2022-04-20T23:42:40Z

src/Extensions/ComposerLoaderExtension.php

@@ -113,7 +101,7 @@ public function onAfterBuild()
        // Mock COMPOSER_HOME if it's not defined already. Composer requires one of the two to be set.
        if (!Environment::getEnv('COMPOSER_HOME')) {
            $home = Environment::getEnv('HOME');
-            if (!$home || !is_dir($home) || !is_writable($home)) {
+            if (!$home || !is_dir($home ?? '') || !is_writable($home ?? '')) {


!$home should short circuit on null, so the null coalescing operator isn't necessary.

GuySartorelli · 2022-04-20T23:43:22Z

src/Extensions/ComposerLoaderExtension.php

@@ -53,7 +51,7 @@ public function getPackages(array $allowedTypes = null)
        $repository = $this->getRepository();
        foreach ($repository->getPackages() as $package) {
            // Filter out packages that are not "allowed types"
-            if (is_array($allowedTypes) && !in_array($package->getType(), $allowedTypes)) {
+            if (is_array($allowedTypes) && !in_array($package->getType(), $allowedTypes ?? [])) {


if is_array($allowedTypes) is false, then we won't get to the in_array() call, so the null coalescing operator isn't necessary.

GuySartorelli · 2022-04-20T23:46:40Z

src/Extensions/ComposerLoaderExtension.php

@@ -130,7 +118,7 @@ public function onAfterBuild()
        $this->setComposer($composer);

        if ($originalDir !== BASE_PATH) {
-            chdir($originalDir);
+            chdir($originalDir ?? '');


$originalDir is either a string or false, never null.

Suggested change

chdir($originalDir ?? '');

chdir($originalDir ?: '');

GuySartorelli · 2022-04-20T23:51:30Z

src/UpdateChecker.php

@@ -117,7 +116,7 @@ protected function findLatestPackage(
        }

        $targetVersion = null;
-        if (0 === strpos($installedVersion, 'dev-')) {
+        if (0 === strpos($installedVersion ?? '', 'dev-')) {


Noting here for posterity:
$installedVersion comes from PackageInterface::getPrettyVersion(). This is strongly typed from 2.3.0 to return a string - but not in 2.0.0.

GuySartorelli · 2022-04-20T23:58:49Z

src/Extensions/ComposerLoaderExtension.php

     */
-    protected function getInstalledConstraint(BaseRepository $repository, $packageName)
+    protected function getInstalledConstraint(InstalledRepository $repository, string $packageName): string


Link::getPrettyConstraint() can return null in composer 2.0.0

Suggested change

protected function getInstalledConstraint(InstalledRepository $repository, string $packageName): string

protected function getInstalledConstraint(InstalledRepository $repository, string $packageName)

I've added $link->getPrettyConstraint() ?? '' so that it's always a string

GuySartorelli · 2022-04-20T23:59:00Z

src/Extensions/ComposerLoaderExtension.php

-     *
-     * @param BaseRepository $repository
-     * @param string $packageName
-     * @return string


Why are we removing the PHPDoc typings here (and, I've just noticed, elsewhere as well)?

Because this is going to be a new major, and I've switched to using proper method type hints

Is the intention that anywhere we have type hints we should remove phpdoc typings? I'm generally in favour of that wherever the phpdoc doesn't provide additional context such as here - but in that case we should probably do it as a blanket action everywhere, yes?
I think we should probably leave the phpdocs alone for this PR, since making graphql4 strongly typed is already another load of work that needs to be done, and it should be done consistently (either leaving/updating them or removing them).

everywhere, yes?

No. Most of Silverstripe was made for PHP 5, is dynamically typed and relys on docblocks for typehinting.

There are plenty of example where there are strongly typed methods with docblocks, though that was done when PHP 5.6 was still supported. It is no longer supported. However removing the old docblocks on a case by case basis right now is very low priority. I'm sure we'll do an automated bulk cleanup at some point in the future.

Where new API is created (which is what is happening here, this is for a new major after all), I'm using strong typing and not using docblock types

I had meant "everywhere" as in "everywhere in this module where new API is provided" so I think we're on the same page there.

Also for some reason when I made that comment my brain was telling me this was on silverstripe/graphql which it definitely isn't. 😅Sorry about that. Yup I'm okay with this.

GuySartorelli

Ran locally, worked great. For any private repositories, a short line is added to the messages for the queued job as below, and that repository doesn't have its "latest" or "available" columns populated in the report.

[DEBUG] Could not authenticate against github.com

LGTM. I don't have access to merge this, though.

emteknetnz · 2022-04-22T00:41:17Z

@spekulatius Just to let you know we're going to merge this and tag a new major, let us know if this is an issue. If we haven't heard from you within a few days we'll assume this is ok and go ahead with it.

spekulatius · 2022-04-22T11:09:31Z

Sounds good to me @emteknetnz

This was referenced Apr 20, 2022

DEP Allow composer 2 #61

Closed

PHP 8.1 compatibility silverstripe/silverstripe-framework#10250

Closed

emteknetnz force-pushed the pulls/master/composer2 branch 4 times, most recently from 7c4dade to aeb9425 Compare April 20, 2022 00:59

emteknetnz mentioned this pull request Apr 20, 2022

DEP Use v3 of silverstripe-composer-update-checker silverstripe/recipe-reporting-tools#26

Merged

emteknetnz force-pushed the pulls/master/composer2 branch from aeb9425 to f6730af Compare April 20, 2022 03:48

emteknetnz mentioned this pull request Apr 20, 2022

ENH PHP 8.1 compatibility #60

Closed

emteknetnz marked this pull request as ready for review April 20, 2022 05:07

This was referenced Apr 20, 2022

DEP Allow installation of composer v2 #53

Closed

Update to support Composer 2.0 #50

Open

GuySartorelli requested changes Apr 21, 2022

View reviewed changes

NEW Use composer 2

dcd4d1d

emteknetnz force-pushed the pulls/master/composer2 branch from f6730af to dcd4d1d Compare April 21, 2022 02:51

GuySartorelli approved these changes Apr 21, 2022

View reviewed changes

emteknetnz merged commit 416a550 into bringyourownideas:master Apr 26, 2022

emteknetnz deleted the pulls/master/composer2 branch April 26, 2022 01:16

emteknetnz added a commit that referenced this pull request Apr 26, 2022

NEW Use composer 2 (#62)

ab967ee

emteknetnz added a commit that referenced this pull request Apr 26, 2022

NEW Use composer 2 (#62)

cf8e387

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NEW Use composer 2 #62

NEW Use composer 2 #62

emteknetnz commented Apr 20, 2022 •

edited

emteknetnz commented Apr 20, 2022

GuySartorelli Apr 20, 2022

emteknetnz Apr 21, 2022

GuySartorelli Apr 20, 2022

GuySartorelli Apr 20, 2022

emteknetnz Apr 21, 2022

GuySartorelli Apr 20, 2022

GuySartorelli Apr 20, 2022

emteknetnz Apr 21, 2022

GuySartorelli Apr 20, 2022

emteknetnz Apr 21, 2022

GuySartorelli Apr 21, 2022

emteknetnz Apr 21, 2022 •

edited

GuySartorelli Apr 21, 2022

GuySartorelli left a comment

emteknetnz commented Apr 22, 2022

spekulatius commented Apr 22, 2022

	protected function getInstalledConstraint(InstalledRepository $repository, string $packageName): string
	protected function getInstalledConstraint(InstalledRepository $repository, string $packageName)

NEW Use composer 2 #62

NEW Use composer 2 #62

Conversation

emteknetnz commented Apr 20, 2022 • edited

emteknetnz commented Apr 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emteknetnz Apr 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli left a comment

Choose a reason for hiding this comment

emteknetnz commented Apr 22, 2022

spekulatius commented Apr 22, 2022

emteknetnz commented Apr 20, 2022 •

edited

emteknetnz Apr 21, 2022 •

edited