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

Array to string conversion #88

Closed
jamesdevine opened this issue Aug 20, 2020 · 3 comments
Closed

Array to string conversion #88

jamesdevine opened this issue Aug 20, 2020 · 3 comments

Comments

@jamesdevine
Copy link

jamesdevine commented Aug 20, 2020

The following code in src/Composer/ScriptHandler.php causes an "Array to string conversion" error if a command doesn't successfully run.

Line 94:

if (!$process->isSuccessful()) {
     throw new \RuntimeException(sprintf('An error occurred while executing the "%s" command: %s', $cmd, $process->getErrorOutput()));
}

$cmd is an array and not a string:

Line 60:
private static function executeCommand(array $cmd, Event $event, string $env = 'prod'): void

@fritzmg
Copy link
Contributor

fritzmg commented Aug 20, 2020

@richardhj encountered the same problem.

$cmd is an array and not a string:

Technically it shouldn't be, because within composer a much older version of symfony/process is in use. See contao/contao#1956

@m-vo
Copy link
Member

m-vo commented Aug 20, 2020

Could anyone of you run the following? You need to set a breakpoint in the constructor of Process inside the composer.phar + have xdebug enabled on the CLI.

COMPOSER_ALLOW_XDEBUG=1 php -dxdebug.remote_enable=1 -dxdebug.remote_autostart=1 composer.phar run post-update-cmd

I'm curious about the used process version and the callstack. All process versions up to symfony/process v5.0 allow passing a string and contain the method setCommandline(). We use this to identify if we need to implode the string but it seems there is something going wrong.

@leofeyer
Copy link
Member

See contao/contao#2184

leofeyer pushed a commit to contao/contao that referenced this issue Aug 25, 2020
…essage (see #2184)

Description
-----------

| Q                | A
| -----------------| ---
| Fixed issues     | Fixes contao/manager-bundle#88
| Docs PR or issue | -

There is indeed a missing implode at this place.

Commits
-------

83c3889 add a missing array->string conversion in the exception message
leofeyer pushed a commit that referenced this issue Aug 25, 2020
…essage (see #2184)

Description
-----------

| Q                | A
| -----------------| ---
| Fixed issues     | Fixes #88
| Docs PR or issue | -

There is indeed a missing implode at this place.

Commits
-------

83c3889e add a missing array->string conversion in the exception message
leofeyer pushed a commit to contao/contao that referenced this issue Jan 15, 2021
Description
-----------

| Q                | A
| -----------------| ---
| Fixed issues     | Fixes ... let's see 😉 
| Docs PR or issue | todo

Our `ScriptHandler` is currently running inside Composer which is problematic (see #1956, #1964, contao/contao-manager#582, contao/manager-bundle#88, ...) mostly due to the fact that we're then sharing Composer's already loaded 3rd party requirements in another/unpredictable version than we're requiring them.

Imo the only way to properly solve this is by using a dedicated binary (compare to  composer/composer#6738 (comment)). I previously thought this would not work because paths in the `composer.json` script section are not normalized and therefore only work in all OS but Windows or just in Windows depending on the type of slashes used. *But:* Composer pushes the `vendor/bin` dir [onto the PATH environment](https://getcomposer.org/doc/articles/scripts.md#writing-custom-commands) so it's possible to have a usage without any slashes. 😄 

That said, here is how this POC works:
* The `ScriptHandler` now works without a `Composer\Script\Event`. Unfortunately it cannot find out the original command flags or if the IO is decorated or not.
* It therefore allows passing additional flags that are appended to each executed command (like `--ansi`, `-vvv`).
* If you still call it from within Composer, a legacy routine kicks in, informs the user to change the script definition and starts a process with the new binary as a fallback.
* The binary simply requires the `autoload.php` and then calls the `ScriptHandler` and passes its arguments on as command flags.

The managed edition `composer.json` should be changed to this:
```json
    "scripts": {
        "post-install-cmd": [
            "contao-script-handler"
        ],
        "post-update-cmd": [
            "contao-script-handler"
        ]
    }
```

Note: I ignored the fact that there are other commands than `initializeApplication` for now. If we want to go down this route we should discuss the API of the binary (e.g. if it should allow calling individual bits). It should eventually go into the `core-bundle` then if it has a purpose outside the ME. Plus, there also is a another `ScriptHandler` in the `core-bundle`, which does not seem to be used anymore which we should probably take care of.

Commits
-------

8abcc0e add command to initialize application
ffb2de2 deprecate script handler + redirect to command
58bc88b improve testability
7db2650 add unit tests
cba6dd1 fix float conversion
9c8d14e fix return type annotation
8680777 fix unit tests under windows
7ed330d make yamllint happy
595bfed prepend php executable
0611b21 adjust timeouts
f17eab5 add more tests
b495095 simplify unit tests
24077c0 make web dir a relative path
b55d021 always purge cache (so that the new command will be found)
d7ec0ab use real console path instead of symlinked/proxy one
fa7deaa setup a script handler binary
6df4797 inline bootstrap.php
8b741ad drop unnecessary variable + type hint
a16ac29 CS
c5a6ca4 Merge branch 'master' into feature/script-handler-bin
8822e58 rename contao-script-handler to contao-setup
29b568b drop obsolete cache purge stage
2365edb re-add outputting 'contao:migrate' info
dfeb3f3 rename command
87c2019 drop filesystem usage in command (cache gets already purged in binary)
eddc067 fix type annotation
a5fa602 display startup errors when running with -vvv
34e1c41 adjust wording
c3750a8 Fix indentation

Co-authored-by: Martin Auswöger <martin@auswoeger.com>
2e06a6f use a closure for process creation
1b6d783 Merge branch 'feature/script-handler-bin' of github.com:m-vo/contao into feature/script-handler-bin
dd98b28 Adjust the deprecation messages
c4f0fb0 Update manager-bundle/src/Resources/config/commands.yml

Co-authored-by: Leo Feyer <github@contao.org>
845b0a0 drop obsolete usage statement
287776b fix createProcessHandler usage in tests
acce470 Do not prepend vendor/bin/ to the binary
0d6de31 fix the unit tests on windows
leofeyer pushed a commit that referenced this issue Jan 15, 2021
Description
-----------

| Q                | A
| -----------------| ---
| Fixed issues     | Fixes ... let's see 😉 
| Docs PR or issue | todo

Our `ScriptHandler` is currently running inside Composer which is problematic (see #1956, #1964, contao/contao-manager#582, #88, ...) mostly due to the fact that we're then sharing Composer's already loaded 3rd party requirements in another/unpredictable version than we're requiring them.

Imo the only way to properly solve this is by using a dedicated binary (compare to  composer/composer#6738 (comment)). I previously thought this would not work because paths in the `composer.json` script section are not normalized and therefore only work in all OS but Windows or just in Windows depending on the type of slashes used. *But:* Composer pushes the `vendor/bin` dir [onto the PATH environment](https://getcomposer.org/doc/articles/scripts.md#writing-custom-commands) so it's possible to have a usage without any slashes. 😄 

That said, here is how this POC works:
* The `ScriptHandler` now works without a `Composer\Script\Event`. Unfortunately it cannot find out the original command flags or if the IO is decorated or not.
* It therefore allows passing additional flags that are appended to each executed command (like `--ansi`, `-vvv`).
* If you still call it from within Composer, a legacy routine kicks in, informs the user to change the script definition and starts a process with the new binary as a fallback.
* The binary simply requires the `autoload.php` and then calls the `ScriptHandler` and passes its arguments on as command flags.

The managed edition `composer.json` should be changed to this:
```json
    "scripts": {
        "post-install-cmd": [
            "contao-script-handler"
        ],
        "post-update-cmd": [
            "contao-script-handler"
        ]
    }
```

Note: I ignored the fact that there are other commands than `initializeApplication` for now. If we want to go down this route we should discuss the API of the binary (e.g. if it should allow calling individual bits). It should eventually go into the `core-bundle` then if it has a purpose outside the ME. Plus, there also is a another `ScriptHandler` in the `core-bundle`, which does not seem to be used anymore which we should probably take care of.

Commits
-------

8abcc0e7 add command to initialize application
ffb2de27 deprecate script handler + redirect to command
58bc88bc improve testability
7db26501 add unit tests
cba6dd17 fix float conversion
9c8d14e6 fix return type annotation
8680777b fix unit tests under windows
7ed330df make yamllint happy
595bfed3 prepend php executable
0611b214 adjust timeouts
f17eab53 add more tests
b4950958 simplify unit tests
24077c04 make web dir a relative path
b55d0215 always purge cache (so that the new command will be found)
d7ec0ab6 use real console path instead of symlinked/proxy one
fa7deaa4 setup a script handler binary
6df47977 inline bootstrap.php
8b741ad4 drop unnecessary variable + type hint
a16ac29c CS
c5a6ca48 Merge branch 'master' into feature/script-handler-bin
8822e584 rename contao-script-handler to contao-setup
29b568be drop obsolete cache purge stage
2365edb9 re-add outputting 'contao:migrate' info
dfeb3f34 rename command
87c20190 drop filesystem usage in command (cache gets already purged in binary)
eddc067d fix type annotation
a5fa6022 display startup errors when running with -vvv
34e1c411 adjust wording
c3750a8e Fix indentation

Co-authored-by: Martin Auswöger <martin@auswoeger.com>
2e06a6ff use a closure for process creation
1b6d783e Merge branch 'feature/script-handler-bin' of github.com:m-vo/contao into feature/script-handler-bin
dd98b28b Adjust the deprecation messages
c4f0fb06 Update manager-bundle/src/Resources/config/commands.yml

Co-authored-by: Leo Feyer <github@contao.org>
845b0a05 drop obsolete usage statement
287776bc fix createProcessHandler usage in tests
acce470c Do not prepend vendor/bin/ to the binary
0d6de31a fix the unit tests on windows
AlexejKossmann pushed a commit to AlexejKossmann/contao that referenced this issue Apr 6, 2021
…essage (see contao#2184)

Description
-----------

| Q                | A
| -----------------| ---
| Fixed issues     | Fixes contao/manager-bundle#88
| Docs PR or issue | -

There is indeed a missing implode at this place.

Commits
-------

83c3889 add a missing array->string conversion in the exception message
AlexejKossmann pushed a commit to AlexejKossmann/contao that referenced this issue Apr 6, 2021
Description
-----------

| Q                | A
| -----------------| ---
| Fixed issues     | Fixes ... let's see 😉
| Docs PR or issue | todo

Our `ScriptHandler` is currently running inside Composer which is problematic (see contao#1956, contao#1964, contao/contao-manager#582, contao/manager-bundle#88, ...) mostly due to the fact that we're then sharing Composer's already loaded 3rd party requirements in another/unpredictable version than we're requiring them.

Imo the only way to properly solve this is by using a dedicated binary (compare to  composer/composer#6738 (comment)). I previously thought this would not work because paths in the `composer.json` script section are not normalized and therefore only work in all OS but Windows or just in Windows depending on the type of slashes used. *But:* Composer pushes the `vendor/bin` dir [onto the PATH environment](https://getcomposer.org/doc/articles/scripts.md#writing-custom-commands) so it's possible to have a usage without any slashes. 😄

That said, here is how this POC works:
* The `ScriptHandler` now works without a `Composer\Script\Event`. Unfortunately it cannot find out the original command flags or if the IO is decorated or not.
* It therefore allows passing additional flags that are appended to each executed command (like `--ansi`, `-vvv`).
* If you still call it from within Composer, a legacy routine kicks in, informs the user to change the script definition and starts a process with the new binary as a fallback.
* The binary simply requires the `autoload.php` and then calls the `ScriptHandler` and passes its arguments on as command flags.

The managed edition `composer.json` should be changed to this:
```json
    "scripts": {
        "post-install-cmd": [
            "contao-script-handler"
        ],
        "post-update-cmd": [
            "contao-script-handler"
        ]
    }
```

Note: I ignored the fact that there are other commands than `initializeApplication` for now. If we want to go down this route we should discuss the API of the binary (e.g. if it should allow calling individual bits). It should eventually go into the `core-bundle` then if it has a purpose outside the ME. Plus, there also is a another `ScriptHandler` in the `core-bundle`, which does not seem to be used anymore which we should probably take care of.

Commits
-------

8abcc0e add command to initialize application
ffb2de2 deprecate script handler + redirect to command
58bc88b improve testability
7db2650 add unit tests
cba6dd1 fix float conversion
9c8d14e fix return type annotation
8680777 fix unit tests under windows
7ed330d make yamllint happy
595bfed prepend php executable
0611b21 adjust timeouts
f17eab5 add more tests
b495095 simplify unit tests
24077c0 make web dir a relative path
b55d021 always purge cache (so that the new command will be found)
d7ec0ab use real console path instead of symlinked/proxy one
fa7deaa setup a script handler binary
6df4797 inline bootstrap.php
8b741ad drop unnecessary variable + type hint
a16ac29 CS
c5a6ca4 Merge branch 'master' into feature/script-handler-bin
8822e58 rename contao-script-handler to contao-setup
29b568b drop obsolete cache purge stage
2365edb re-add outputting 'contao:migrate' info
dfeb3f3 rename command
87c2019 drop filesystem usage in command (cache gets already purged in binary)
eddc067 fix type annotation
a5fa602 display startup errors when running with -vvv
34e1c41 adjust wording
c3750a8 Fix indentation

Co-authored-by: Martin Auswöger <martin@auswoeger.com>
2e06a6f use a closure for process creation
1b6d783 Merge branch 'feature/script-handler-bin' of github.com:m-vo/contao into feature/script-handler-bin
dd98b28 Adjust the deprecation messages
c4f0fb0 Update manager-bundle/src/Resources/config/commands.yml

Co-authored-by: Leo Feyer <github@contao.org>
845b0a0 drop obsolete usage statement
287776b fix createProcessHandler usage in tests
acce470 Do not prepend vendor/bin/ to the binary
0d6de31 fix the unit tests on windows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants