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

PHP8: add crashing nullable type case #5400

Closed
wants to merge 1 commit into from
Closed

PHP8: add crashing nullable type case #5400

wants to merge 1 commit into from

Conversation

keradus
Copy link
Member

@keradus keradus commented Dec 28, 2020 •
edited
Loading

anyone willing to take over?

@keradus keradus mentioned this pull request Dec 28, 2020
Merged
6 tasks
@keradus keradus marked this pull request as draft December 28, 2020 17:23
keradus
keradus commented Dec 28, 2020
View reviewed changes
tests/Fixtures/Integration/misc/property_promotion_with_nullable_type.test
Integration test to verify that property promotion with nullable type does not crash the Tokenizer.
--RULESET--
{
"ternary_to_elvis_operator": true
Copy link
Member Author

Choose a reason for hiding this comment

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

random rule. not related to the problem...
most probably the test case can be moved directly to TokenizerTest class

@coveralls
Copy link

coveralls commented Dec 28, 2020 •
edited
Loading

Coverage Status

Coverage remained the same at 91.369% when pulling fd47229 on keradus:2.17_php8_nullable into bb6ee85 on FriendsOfPHP:2.17.

@Wirone
Copy link
Member

Wirone commented Dec 30, 2020

Most probably related to #5404 🙂

@keradus
Copy link
Member Author

keradus commented Dec 30, 2020

lets restart the CI and confirm that assumption. but looks promising indeed

@Wirone
Copy link
Member

Wirone commented Dec 31, 2020

In general, after #5404 tokenizer works properly with property promotion and nullable types and as long as test cases have proper @require with PHP version, it will work as expected. It will fail like Parse error: syntax error, unexpected 'private' (T_PRIVATE), expecting variable (T_VARIABLE) in runtime <8.0, but this fail is from AbstractFixerTestCase::lintSource() just before tokenization.

Side questions:

  • Why AbstractFixerTestCase::lintSource() catches \Exception, not \Throwable?
  • What about failed tests, why check is marked as passed 🤔 (out of curiosity)?

@keradus
Copy link
Member Author

keradus commented Jan 4, 2021

It will fail like Parse error: syntax error, unexpected 'private' ...

@Wirone , can you please point to cases where @requires PHP 8.0 is missing? we should add it for code-samples relying on PHP 8 syntax.

Why AbstractFixerTestCase::lintSource() catches \Exception, not \Throwable?

probably code create before Throwable was introduced. Do we have a valid case when Throwable is raised and we should catch it, rather than fix code leading to this exception?

  • What about failed tests, why check is marked as passed 🤔 (out of curiosity)?

I did rebase and cannot see the link content anymore. Anyway, could be related to GitHub Action configuration: continue-on-error: ${{ matrix.php-version == '8.0' }}

@keradus
Copy link
Member Author

keradus commented Jan 6, 2021

works smoothly now, looks like #5404 saved the day indeed!

OK, but incomplete, skipped, or risky tests!
Tests: 24609, Assertions: 540397, Skipped: 180, Incomplete: 26.

@keradus keradus closed this Jan 6, 2021
@keradus keradus deleted the 2.17_php8_nullable branch January 6, 2021 00:31
keradus added a commit that referenced this pull request Jan 18, 2021
@keradus
minor #5399 Add PHP8 integration test (keradus)
f209e6d 
This PR was squashed before being merged into the 2.17 branch.

Discussion
----------

Add PHP8 integration test

ref #4702

There are ~3~ 4 cases still crashing in the spec files:
- [x] nullsafe operator -> PHP 8.0.1 to be released at 7th
- [x] attributes #5406
- [x] union types for method parameters #5405
- [x] union types for class properties #5439

I also found the following issues that I raised as separated PRs:
- [x] ~#5396~ -> #5397
- [x] ~#5400~ -> #5404

Commits
-------

324929f Add PHP8 integration test
@Wirone
Copy link
Member

Wirone commented Jan 20, 2021

Sorry for radio silence, I missed this notification. I did not see any @requires PHP 8.0 missing, I just said that it will fail IF run on the wrong version of PHP. Good to hear that it's working 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@keradus @coveralls @Wirone