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

[CMS-459] Add php 8.1 support #2295

Merged
merged 35 commits into from
Jan 11, 2022
Merged

[CMS-459] Add php 8.1 support #2295

merged 35 commits into from
Jan 11, 2022

Conversation

jabxjab
Copy link
Contributor

@jabxjab jabxjab commented Dec 19, 2021

No description provided.

@jabxjab jabxjab requested a review from a team December 19, 2021 11:01
@@ -63,7 +63,11 @@

if (!getenv('TERMINUS_TESTING_RUNTIME_ENV')) {
// Create a testing runtime multidev environment.
$sitename = TerminusTestTrait::getSiteName();
$sitename = (new class {
Copy link
Member

Choose a reason for hiding this comment

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

There must be a better way to refactor this helper so that we do not need to instantiate an anonymous class just to call the wrapper.

Maybe make a test util class with the useful static methods we need to call from bootstrap. Could move all of the statics from TerminusTestTrait there, optionally with wrappers in TerminusTestTrait, although perhaps this level of cleanup isn't worth the effort.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

100% agree with this. I was not a fan of this TerminusTestTrait from the beginning.
Refactored TerminusTestTrait into TerminusTestBase.

@@ -68,6 +68,7 @@ public function setStatusCodeReason(string $status_code_reason): void
* @param mixed $offset
* @return bool
*/
#[\ReturnTypeWillChange]
Copy link
Member

Choose a reason for hiding this comment

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

Glad to see we won't need separate implementations of this class.

Copy link
Member

@greg-1-anderson greg-1-anderson left a comment

Choose a reason for hiding this comment

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

Converting from a trait to a base class is a good improvement.

@uberhacker
Copy link
Contributor

Merge conflicts? Probably due to #2299.

Sergei Churilo and others added 4 commits January 5, 2022 14:21
@greg-1-anderson
Copy link
Member

Updated dependencies and rebased with HEAD of 3.x.

@jabxjab jabxjab changed the title [CMS-459] Add php 8.1 support [CMS-459] Add php 8.1 and php 8.2-dev support Jan 10, 2022
bin/terminus Outdated
if (version_compare(PHP_VERSION, '7.4.0', '<') === true) {
echo "\n";
echo 'Sorry, your PHP version (' . PHP_VERSION . ') is no longer supported.' . "\n";
echo 'Upgrade to PHP 7.4 or newer to use Terminus 3. For PHP versions prior to 7.4, downgrade to Terminus 2.x.'; echo "\n\n";
exit(1);
} elseif (version_compare(PHP_VERSION, '8.2.0', '>=') === true) {
Copy link
Member

Choose a reason for hiding this comment

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

We should always have some version of PHP that we don't support yet. Is it too early to say that 8.2 stable will run without issue on the next release of Terminus? If not, then let's at a minimum reject 8.3.0+

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@greg-1-anderson Rolled-back to support active php 8.1 only

bin/terminus Outdated

if (version_compare(PHP_VERSION, '8.1.0', '>=') === true) {
// Prevent outputting PHP deprecation messages produced by vendors.
error_reporting(error_reporting() & ~E_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

If the consolidation dependencies have been updated, then we might not need this any longer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@greg-1-anderson Updated dependencies and tested for 8.1 - tests passed. Not needed for PHP 8.1 anymore, so removed.

protected function assertCommandExists(string $commandName)
{
$commandList = $this->terminus('list');
$this->assertStringContainsString($commandName, $commandList);
Copy link
Member

Choose a reason for hiding this comment

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

Might this give a false positive? Maybe run help <commandName> and check for errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@greg-1-anderson Better to ask @kporras07 since this is a code merging change (getting rid of TerminusTestTrait and TerminusUtilsTrait)

@jabxjab jabxjab changed the title [CMS-459] Add php 8.1 and php 8.2-dev support [CMS-459] Add php 8.1 support Jan 11, 2022
} elseif (version_compare(PHP_VERSION, '8.1.0', '>=') === true) {
}

if (!getenv('TERMINUS_ALLOW_UNSUPPORTED_NEWER_PHP') && version_compare(PHP_VERSION, '8.2.0', '>=') === true) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The check for getenv('TERMINUS_ALLOW_UNSUPPORTED_NEWER_PHP') needs to be at the top since any echo would bloat commands' responses.

Copy link
Member

@greg-1-anderson greg-1-anderson left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@jabxjab jabxjab merged commit 52bfa09 into 3.x Jan 11, 2022
@jabxjab jabxjab deleted the feature/cms-459 branch January 11, 2022 14:17
@CdrMarks
Copy link
Contributor

Fixes #2279

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.

4 participants