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

PHP8 support. PHPUnit 9. Update Travis. Update test config #139

Conversation

chrispenny
Copy link
Contributor

@chrispenny chrispenny commented Jun 8, 2022

Fixes #135
Fixes #138

PHP Unit 9

Not too much had to update here. Just the usual missing : void declarations, and I needed to update some deprecated methods and assertContains to the new assetStringContainsString.

Travis update

I'm not 100% across all of the settings that used to be there (in particular the cache dir settings). I've really just grabbed what is the latest "template" from some other Silverstripe modules. 🤞

Updated yml config

Practical test

  • Manually test that project purges URLs when unpublished
  • Manually test that project builds URLs when published
  • Manually test that project rebuilds parent URLs when child is published or unpublished

@chrispenny chrispenny force-pushed the pulls/php-8-unit-9-travis-config branch 3 times, most recently from d4790a5 to 37b0fa4 Compare June 8, 2022 22:04
@chrispenny chrispenny force-pushed the pulls/php-8-unit-9-travis-config branch from 37b0fa4 to 8e1dfb8 Compare June 8, 2022 22:19
@mfendeksilverstripe
Copy link
Collaborator

@chrispenny We're almost there, please add the following config to the phpcs.xml.dist just under the <description> tag.

    <file>src</file>
    <file>tests</file>

@mfendeksilverstripe
Copy link
Collaborator

Here is an example of phpcs file:

https://github.com/silverstripe/silverstripe-versioned-snapshots/blob/1/phpcs.xml.dist

@chrispenny chrispenny force-pushed the pulls/php-8-unit-9-travis-config branch from 9799914 to 170acc0 Compare June 9, 2022 00:42
@mfendeksilverstripe
Copy link
Collaborator

All ticks are green :D Nice one @chrispenny

@chrispenny chrispenny marked this pull request as ready for review June 9, 2022 03:00
@chrispenny
Copy link
Contributor Author

Thank you for all of the help, @mfendeksilverstripe!

I've gone through my own regression tests for the project I'm working on, and I'm feeling pretty happy that the existing features that we rely on are still functioning.

@mfendeksilverstripe
Copy link
Collaborator

Hey @chrispenny , I noticed the following warning in the CI run:

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

This is coming from the PHPUnit 9 and we should be able to fix it by updating the phpunit.xml.dist. I think the XML file has now a different structure and naming conventions but you should be able to run the automatic update via phpunit --migrate-configuration command

@chrispenny
Copy link
Contributor Author

Thanks, @mfendeksilverstripe!

I added allowed plugins (for composer v2), and ran the migration-configuration process.

@emteknetnz emteknetnz changed the base branch from master to 5 June 15, 2022 05:32
@@ -1,57 +1,9 @@
language: php
version: ~> 1.0
Copy link
Member

Choose a reason for hiding this comment

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

Could you replace this with https://github.com/silverstripe/gha-ci - we're replacing travis with this

You may need to do this in a separate PR that we merge before this one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've raised #140

Can this happen after this PR?

Copy link
Member

Choose a reason for hiding this comment

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

yeah after is fine

composer.json Outdated Show resolved Hide resolved
phpunit.xml.dist Outdated Show resolved Hide resolved
tests/php/Publisher/FilesystemPublisherTest.php Outdated Show resolved Hide resolved
"silverstripe/recipe-plugin": true,
"silverstripe/vendor-plugin": true
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the branch-alias from composer.json

I've created new 5 and 5.1 branches from master and retargetted this PR to 5

Please also create a follow up PR to remove the branch alias from the master branch

You'll want to check relevant projects that they're requiring this as ^5 rather than dev-master

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really understand what the consequences of those changes are, but they kinda sound like general house keeping? Does this all need to part of this PHP 8 upgrade PR?

Copy link
Member

@emteknetnz emteknetnz Jun 16, 2022

Choose a reason for hiding this comment

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

Preferably now .. we're targeting this at the 5 branch (which should have always existed), so if we keep the existing branch-alias's in the 5 and master branches there' a chance that requesting 5.x-dev will mistakenly fetch from the master branch when it shouldn't

chrispenny and others added 3 commits June 16, 2022 07:18
Co-authored-by: Steve Boyd <emteknetnz@gmail.com>
Co-authored-by: Steve Boyd <emteknetnz@gmail.com>
Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Happy with changes here

@emteknetnz emteknetnz merged commit 06e83f6 into silverstripe:5 Jun 16, 2022
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.

5.1: SapphireTestState config forces you to prefer-source for development/testing PHP8 Support
3 participants