Skip to content

Update to PHP 8.1#703

Merged
AndrewKostka merged 2 commits into
mainfrom
moveToPHP81
Dec 21, 2023
Merged

Update to PHP 8.1#703
AndrewKostka merged 2 commits into
mainfrom
moveToPHP81

Conversation

@tarrow

@tarrow tarrow commented Dec 5, 2023

Copy link
Copy Markdown
Contributor

As part of this process it was necssary to make some tweaks related to our use of PDO calls. In 8.1 attempting to explicitly commit or rollback a transaciton when none is in progress now errors rather than failing silently[1].

We have a number of these in our wiki deletion code since the altering of the tables automatically commits and ends the transaction.

For this reason we are also unable use the traits that rollback transactions after running tests if the tests themselves are causing a commit. These have been removed too. Unfortunately this adds more places where the tests are no longer idempotent.

[1] php/php-src@990bb34

@m90 m90 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I do understand the change in PDO behavior but I am still a bit puzzled about the changes caused by this.

Comment thread app/Jobs/DeleteWikiDbJob.php Outdated
Comment thread app/Jobs/DeleteWikiDbJob.php Outdated
@m90

m90 commented Dec 11, 2023

Copy link
Copy Markdown
Contributor

We'll also need to update this setting:

php_version: 7.4

@m90

m90 commented Dec 12, 2023

Copy link
Copy Markdown
Contributor

When installing dependencies locally using PHP 8, I will get prompted to update the lock file so we probably need to do this here as well?

Warning: The lock file is not up to date with the latest changes in composer.json. You may be getting outdated dependencies. It is recommended that you run `composer update` or `composer update <package name>`.   

As part of this process it was necssary to make some tweaks related
to our use of PDO calls. In 8.1 attempting to explicitly commit
or rollback a transaciton when none is in progress now errors
rather than failing silently[1].

We have a number of these in our wiki deletion code since the
altering of the tables automatically commits and ends the
transaction.

For this reason we are also unable use the traits that rollback
transactions after running tests if the tests themselves are
causing a commit. These have been removed too. Unfortunately
this adds more places where the tests are no longer idempotent.

[1] php/php-src@990bb34
@m90

m90 commented Dec 19, 2023

Copy link
Copy Markdown
Contributor

I'm still seeing the warning here: #703 (comment) - is this ok / expected or should we also update the lockfile?

@tarrow

tarrow commented Dec 19, 2023

Copy link
Copy Markdown
Contributor Author

When installing dependencies locally using PHP 8, I will get prompted to update the lock file so we probably need to do this here as well?

Warning: The lock file is not up to date with the latest changes in composer.json. You may be getting outdated dependencies. It is recommended that you run `composer update` or `composer update <package name>`.   

Interestingly I don't see this even with no updates using the composer:latest image for running composer install

This is probably true; it's somewhat hard to do this in isolation though; this typically involves pulling in loads of updated dependencies. I'll follow up with another PR for it

@tarrow

tarrow commented Dec 19, 2023

Copy link
Copy Markdown
Contributor Author

#713

@m90

m90 commented Dec 20, 2023

Copy link
Copy Markdown
Contributor

Not entirely sure what the tests failing in #713 mean for this PR. What's your plan for proceeding with this @tarrow ?

@tarrow

tarrow commented Dec 20, 2023

Copy link
Copy Markdown
Contributor Author

I haven't got a solid plan to be honest; I'd be happy to:

  • look at fixing up Run composer update after using php81 #713 and only when that's green move ahead
  • OR move ahead with this patch as is no changes
  • OR look together about why you see composer complaints and I don't

@m90

m90 commented Dec 20, 2023

Copy link
Copy Markdown
Contributor

I have to admit I know too little about PHP and package management to know about the consequences of such a warning being ignored. Could it mean we end up with a incompatible dependency tree, or is it just sth cosmetic?

@tarrow

tarrow commented Dec 20, 2023

Copy link
Copy Markdown
Contributor Author

as I understand it the risk is that you think you have newer dependencies than you do because the lockfile is taking priority over the composer.json. It's cosmetic to the extent that the vendor folder should be functional but you might be confused

@m90

m90 commented Dec 20, 2023

Copy link
Copy Markdown
Contributor

As for why I am seeing the message, this is my setup

➜  api git:(moveToPHP81) composer --version                                                                                                                                                      production
Composer version 2.5.5 2023-03-21 11:50:05
➜  api git:(moveToPHP81) php --version                                                                                                                                                           production
PHP 8.1.19 (cli) (built: Nov  6 2023 14:16:56) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.1.19, Copyright (c) Zend Technologies

I don't run this in Docker but use PHPBrew.

* Run  after using php81

* Use legacy PSR-7 message factories

---------

Co-authored-by: Andrew Kostka <andrew.kostka@wikimedia.de>
@AndrewKostka
AndrewKostka merged commit 8d37f7a into main Dec 21, 2023
@AndrewKostka
AndrewKostka deleted the moveToPHP81 branch December 21, 2023 14:07
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.

3 participants