fix: composer update might cause error "Failed to open directory"

[LeMyst]

Fixes #6832

Description
Exit the function recursiveDelete() if the folder to delete didn't exist.
Fixes #6832

Checklist:

• Securely signed commits
• [?] Component(s) with PHPDoc blocks, only if necessary or adds value
• [?] Unit testing, with >80% coverage
• [?] User guide updated
• [?] Conforms to style guide

[kenjis]

Wouldn't it be better to leave it as an exception since this is an exceptional situation?

Should we throw an appropriate exception?

[MGatner]

It looks like recursiveMirror() will fail if the directory isn't there (not 100% sure on that). If that's the case I'd be in favor of exiting early here, like @kenjis suggests.

[LeMyst]

Wouldn't it be better to leave it as an exception since this is an exceptional situation?

Should we throw an appropriate exception?

It looks like recursiveMirror() will fail if the directory isn't there (not 100% sure on that). If that's the case I'd be in favor of exiting early here, like @kenjis suggests.

Hello @kenjis and @MGatner ,

If the recursiveDelete fails with the exception, the recursiveMirror is not executed and the composer update is not finalized.

It's better to not throw an Exception, to let the recursiveMirror do the job and correctly finish the update.

[sfadschm]

I would agree with @LeMyst, here. The proposed change does not alter the behaviour of the method, but rather provides an early exit to skip the unnecessary generation of Iterator objects if the folder does not exist.

[kenjis]

@LeMyst Please don't use git merge.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#updating-your-branch

[LeMyst]

@LeMyst Please don't use git merge.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#updating-your-branch

I saw that after my action. I will clean my branch tomorrow.

[kenjis]

Yes, if the script is terminated by some reasons right after recursiveDelete() execution,
system/ThirdParty/ does not exist when the next execution of composer update.

[LeMyst]

@LeMyst Please don't use git merge. See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#updating-your-branch

It's cleaned

[kenjis]

How about this?

@@ -126,7 +128,11 @@ final class ComposerScripts
             exit(1);
         }
 
-        @mkdir($targetDir, 0755, true);
+        if (! @mkdir($targetDir, 0755, true)) {
+            echo sprintf('Cannot create the target directory: "%s"' . PHP_EOL, $targetDir);
+
+            exit(1);
+        }
 
         $dirLen = strlen($originDir);
 

[LeMyst]

How about this?

@@ -126,7 +128,11 @@ final class ComposerScripts
             exit(1);
         }
 
-        @mkdir($targetDir, 0755, true);
+        if (! @mkdir($targetDir, 0755, true)) {
+            echo sprintf('Cannot create the target directory: "%s"' . PHP_EOL, $targetDir);
+
+            exit(1);
+        }
 
         $dirLen = strlen($originDir);
 

Does I need to put this change (who's good for me) into this PR?

[kenjis]

I tried to delete ThirdParty and touch a file named ThirdParty, and the function fails because of the trailing '/'.

Without the above code, composer update still raises an error in that case.

[LeMyst]

It's added to the PR, but that raise a question about consistency for the PHP_EOL. Inside or outside the sprintf()?

[kenjis]

Thank you. And good point.
Outside the sprintf() is easier to read?

[paulbalandan]

I would add that to the format string, because it's supposed to be part of the message.

[LeMyst]

Another question I've, it's the difference between composer update and composer install

If I do a composer update, the hook post-update-cmd is used and the ThirdParty dependencies are updated.
But if I do a composer install, if I use the composer.lock on my production line, the ThirdParty folder is not updated.

I think a hook post-install-cmd must be added.

EDIT:

And the comment in ComposerScripts::postUpdate is wrong:

    /**
     * This static method is called by Composer after every update event,
     * i.e., `composer install`, `composer update`, `composer remove`.
     */

[kenjis]

I think a hook post-install-cmd must be added.

No. The third party libraries are updated only when you run composer update.
composer install does not update them. So the hook is not needed.

[LeMyst]

I think a hook post-install-cmd must be added.

No. The third party libraries are updated only when you run composer update. composer install does not update them. So the hook is not needed.

It seems there is different behavior:

• If there is no composer.lock, the composer install run the ComposerScripts::postUpdate
• if you use the composer update, the ComposerScripts::postUpdate is ran too
• but if you use composer install with a composer.lock, the ComposerScripts::postUpdate is not ran and the libraries are not copied/updated in the ThirdParty folder.

You have more informations about this here: https://getcomposer.org/doc/articles/scripts.md#command-events

post-install-cmd: occurs after the install command has been executed with a lock file present.
post-update-cmd: occurs after the update command has been executed, or after the install command has been executed without a lock file present.

So I think a hook on post-install-cmd is necessary to copy the ThirdParty libraries.

[kenjis]

So I think a hook on post-install-cmd is necessary to copy the ThirdParty libraries.

Why?
There is already latest Third party source code in the ThirdParty folder.

[LeMyst]

So I think a hook on post-install-cmd is necessary to copy the ThirdParty libraries.

Why? There is already latest Third party source code in the ThirdParty folder.

If you add dependencies to your composer.json/composer.local.json and you push the composer.lock to your version control. When you update the vendor folder with composer install on your deployment server (production, preproduction, ...), the folder system/ThirdParty will not be updated.

[MGatner]

Correct me if I'm wrong but... if we're using Composer then we don't even need to update ThirdParty because the Composer sources will be used.

[paulbalandan]

To be honest, I think if you are using manual install then you don't need ComposerScripts and the post update hook altogether. It's kinda weird you download the zip manually then use Composer to update. I mean, why not use composer initially?

[LeMyst]

Correct me if I'm wrong but... if we're using Composer then we don't even need to update ThirdParty because the Composer sources will be used.

I don't understand why this is in place, I did not test to remove the ThirdParty folder when you have an up-to-date vendor folder.

To be honest, I think if you are using manual install then you don't need ComposerScripts and the post update hook altogether. It's kinda weird you download the zip manually then use Composer to update. I mean, why not use composer initially?

I am probably not the best example. I build my project around CodeIgniter. I download the zip file and start my project from this, adding (or not) my dependencies to the composer.local.json, etc.
I never really used the AppStarter, so I'm between the two worlds (zip and composer), I don't think I'm a rare case.

[kenjis]

@LeMyst Sorry, if you downloaded the Zip file from this codeigniter4/CodeIgniter4 repository, it was because the official site download link was wrong in the past. Correct zip file is one from codeigniter4/framework like https://api.github.com/repos/codeigniter4/framework/zipball/v4.2.10

If you use Zip installation, you need to replace system folder manually. And you don't need to install the third party packages via Composer. See https://codeigniter4.github.io/CodeIgniter4/installation/installing_manual.html#installing-manual-upgrading

But If you installed the package with Composer, the Composer package will be autoloaded by CI4's autoloader. ThirdParty/ source code is not used.

If you use Composer, we recommend you don't use Zip installation.

I recommend you move to Adding CodeIgniter4 to an Existing Project. Then you can update system folder with composer update. Your composer.json file will be similar to appstar's: https://github.com/codeigniter4/appstarter/blob/master/composer.json that does not have post-update-cmd.

The ComposerScripts.php is for framework maintainers, not for end users.

[LeMyst]

@LeMyst Sorry, if you downloaded the Zip file from this codeigniter4/CodeIgniter4 repository, it was because the official site download link was wrong in the past. Correct zip file is one from codeigniter4/framework like https://api.github.com/repos/codeigniter4/framework/zipball/v4.2.10

If you use Zip installation, you need to replace system folder manually. And you don't need to install the third party packages via Composer. See https://codeigniter4.github.io/CodeIgniter4/installation/installing_manual.html#installing-manual-upgrading

But If you installed the package with Composer, the Composer package will be autoloaded by CI4's autoloader. ThirdParty/ source code is not used.

If you use Composer, we recommend you don't use Zip installation.

I recommend you move to Adding CodeIgniter4 to an Existing Project. Then you can update system folder with composer update. Your composer.json file will be similar to appstar's: https://github.com/codeigniter4/appstarter/blob/master/composer.json that does not have post-update-cmd.

The ComposerScripts.php is for framework maintainers, not for end users.

OK, I will do this next time.

Maybe remove the composer.json from the framework release if it's not intended to be used.

Otherwise, I think we can close this PR if the issue #6832 is solved?

[kenjis]

Maybe remove the composer.json from the framework release if it's not intended to be used.

If devs want to write PHPUnit tests, they need composer.json to install PHPUnit.
After all many devs need composer.json.

We actually recommend Composer installation.

@codeigniter4/core-team
But it seems we do not need the following in codeigniter4/framework composer.json:

        "post-update-cmd": [
            "CodeIgniter\\ComposerScripts::postUpdate"
        ],