Skip to content

Issue #23: Update to react/http major version 1. #24

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

Merged

Conversation

mookman288
Copy link
Contributor

@mookman288 mookman288 commented Oct 2, 2020

Hi,

On issue#23, I've completed the major version upgrade from 0.8.7 to 1 (technically minor version 1.1.0 because of caret versioning.) I found that no changes needed to be made except for changing the namespace for ServerRequest. This was not listed in the migration guide as breaking.

Here were the local test results:

λ composer unit
> composer install --ansi -n -q
> phpunit --colors=always -c phpunit.xml.dist
PHPUnit 8.5.8 by Sebastian Bergmann and contributors.

......                                                              6 / 6 (100%)

Time: 1.54 seconds, Memory: 8.00 MB

OK (6 tests, 7 assertions)
λ composer run-script qa-contrib --timeout=0
> composer install --ansi -n -q
> parallel-lint --exclude vendor .
PHP 7.3.5 | 10 parallel jobs
..                                                           2/2 (100 %)


Checked 2 files in 0.3 seconds
No syntax error found

I found, on Windows, the following error during qa-contrib:

Script php-cs-fixer fix --config=.php_cs --ansi --dry-run --diff --verbose --allow-risky=yes --show-progress=estimating handling the cs event returned with error code 8
Script @cs was called via qa-all
Script @qa-all was called via qa-contrib

However, when running the php-cs-fixer from the bat file, it ran fine:

vendor\bin\php-cs-fixer.bat  fix --config=.php_cs --ansi --dry-run --diff --verbose --allow-risky=yes --show-progress=estimating

I'm not confident that these were the only changes that needed to be made to pass tests, so I won't continue making PRs on the other repositories you have until this one passes.

I would also suggest making changes to the README, since not all operating systems can use the Makefile.

@WyriHaximus
Copy link
Owner

Hi,

On issue#23, I've completed the major version upgrade from 0.8.7 to 1 (technically minor version 1.1.0 because of caret versioning.) I found that no changes needed to be made except for changing the namespace for ServerRequest. This was not listed in the migration guide as breaking.

Here were the local test results:

λ composer unit
> composer install --ansi -n -q
> phpunit --colors=always -c phpunit.xml.dist
PHPUnit 8.5.8 by Sebastian Bergmann and contributors.

......                                                              6 / 6 (100%)

Time: 1.54 seconds, Memory: 8.00 MB

OK (6 tests, 7 assertions)
λ composer run-script qa-contrib --timeout=0
> composer install --ansi -n -q
> parallel-lint --exclude vendor .
PHP 7.3.5 | 10 parallel jobs
..                                                           2/2 (100 %)


Checked 2 files in 0.3 seconds
No syntax error found

I found, on Windows, the following error during qa-contrib:

Script php-cs-fixer fix --config=.php_cs --ansi --dry-run --diff --verbose --allow-risky=yes --show-progress=estimating handling the cs event returned with error code 8
Script @cs was called via qa-all
Script @qa-all was called via qa-contrib

However, when running the php-cs-fixer from the bat file, it ran fine:

vendor\bin\php-cs-fixer.bat  fix --config=.php_cs --ansi --dry-run --diff --verbose --allow-risky=yes --show-progress=estimating

I'm not confident that these were the only changes that needed to be made to pass tests, so I won't continue making PRs on the other repositories you have until this one passes.

I would also suggest making changes to the README, since not all operating systems can use the Makefile.

Just created WyriHaximus/php-test-utilities#274 for that. Been working on a next generation of QA tool chain for my packages, but haven't updated this package yet. Which also solves most of the things you've mentioned here. And since you have the first job on travis succeeding that's good enough for me. Thank you for creating the time to put this PR together 👍 .

@WyriHaximus WyriHaximus merged commit 444a1e4 into WyriHaximus:master Oct 2, 2020
WyriHaximus added a commit that referenced this pull request Oct 2, 2020
Issue #24: Update the contribution document to clarify the use of the Makefile.
@mookman288
Copy link
Contributor Author

Do you want to wait on that package before updating the other projects, or would what I have completed here be sufficient for those projects as well?

@WyriHaximus
Copy link
Owner

Trying use that package as a central place for all improvements and thread spread it from there. If you want, you can also add it there :).

@mookman288 mookman288 deleted the Issues/23-update-react-http branch October 2, 2020 20:01
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

Successfully merging this pull request may close these issues.

2 participants