XDMoD 11.0 PHP 7.4 Changes #1806

ryanrath · 2024-01-08T18:43:18Z

Note: This PR has been updated to require PHP 7.4 due to there being no support for 8.0. A separate PR has been made / is being tested for PHP 8.1

Description

.circleci/config.yml:
- Added additional steps for updating PHP to 8.0 & installing the required system level dependencies.
- Due to the upgrade in SimpleSAML & PHP8 deprecating Assertions we need to remove this line from SimpleSAMLs _include.php file.
bin/acl-config:
- the signature for fwrite() has changed in PHP8.0, that change is reflected here.
classes/DataWarehouse/Export/RealmManager.php:
- export: PHP8 enforces that you can't have optional function arguments ( arguments w/ defaults ) before required ones.
classes/DataWarehouse/Visualization.php:
- PHP8 is more strict w/ type conversions so in order for the switch statement to work correctly I've added an int cast.
classes/ETL/Aggregator/pdoAggregator.php:
- Another instance of "PHP 8 is more strict w/ type conversions", in this instance $firstPeriod was assumed to be a bool value but it wasn't always. The updated code takes this into account.
classes/ETL/DataEndpoint/aStructuredFile.php:
- not having continue 2; here generated PHP warnings.
classes/ETL/Ingestor/HpcdbHostsIngestor.php:
- Another "PHP8 is more strict...", the transform() function did not fully conform to the parent definintion, now it does.
classes/ETL/aRdbmsDestinationAction.php:
- There were instances in which the this if statement failed when it should pass due to intermediary array entries not being present / not having the expected type. The updated if statement behaves as expected.
classes/Realm/GroupBy.php:
- basically the same reason as the changes to classes/ETL/DataEndpoint/aStructuredFile.php, warnings were generated for these lines.
composer.json:
- Updated the required PHP version to 8.0
- Update simplesamlphp to 1.16 ( this is the latest version that we can update to without updating Symfony, we can update further once the rest stack updates go in ).
tests/artifacts/xdmod/**:
- Updated sem_avg_cpu_hours values to include another decimal point of value to reflect what PHP8 provides.
tests/ci/bootstrap.sh
- There are additional DB setup steps added so that everything will work correctly ( including adding the new sql_mode= )
tests/ci/samlSetup.sh
- sed'ing out the SimpleSAML Assertion stuff as it's no longer supported in PHP8 and will cause exceptions.
tests/[component|integration|regression|unit]/**:
- We've updated to PHPUnit 9.x and there are a lot of changes required to make things work with th. new version. In general all of these changes are to handle PHP 9 code being updated and how that interacts with PHP8 being more strict about basically everything.

Motivation and Context

We'd like to move with the times and start using officially supported versions of PHP. This is the first step on that path.

Tests performed

All Automated Tests

Checklist:

The pull request description is suitable for a Changelog entry
The milestone is set correctly on the pull request
The appropriate labels have been added to the pull request

classes/DB/PDODBMultiIngestor.php

.circleci/config.yml

tests/ci/samlSetup.sh

jpwhite4 · 2024-05-20T15:29:24Z

classes/ETL/Aggregator/pdoAggregator.php

+
+        // There are instances where $firstPeriod is a bool not an array ( when $aggregationPeriodList is empty or at the end of the array)
+        // the following code takes that into account.
+        $periodSize = 1;


Is 1 the appropriate default value for periodSize? If aggregationPeriodList is empty then why is periodSize not 0. Similarly if aggregationPeriodList is at the end then the periodSize would be 0 too?

The tldr;
I figured that setting $periodSize = 1 by default meant that $batchSlizeSize wasn't (as) meaningless when determining whether or not to enable batch aggregation. Which I guess boils down to whether or not it matters if BatchAggregation is enabled when there are no dirty aggregation periods.

the not-tldr;
So $periodSize's is strictly being used to determine whether or not to enable Batch Aggregation, the statement of note is:

// Default Values $this->options->enable_batch_aggregation = true; $batchSliceSize = $this->options->batch_aggregation_periods_per_batch = 10; $this->options->batch_aggregation_min_num_periods = 25; $this->options->batch_aggregation_max_num_days_per_batch = 300; // Calculated Values $numAggregationPeriods = count($aggregationPeriodList); // Statement $enableBatchAggregation = $this->options->enable_batch_aggregation && $numAggregationPeriods >= $this->options->batch_aggregation_min_num_periods && ($periodSize * $batchSliceSize) <= $this->options->batch_aggregation_max_num_days_per_batch; // Which basically boils down to: true && $numAggregationPeriods >= 25 && ($periodSize * 10) <= 300; // The result of which basically does some re-jiggering of of which / how tables are used in the aggregate query.

Which I guess is a really long way of saying... 0 works for me

jpwhite4 · 2024-05-20T15:31:06Z

docs/upgrade.md

+that are outside the normal installation of the rpm and `xdmod-upgrade`. Below you will find the different upgrade scenarios
+that exist with steps to accommodate all of them:
+
+### Server: EL7, XDMoD: 10.5, PHP: 7.2


EL7 is not php 7.2

It is if you have 10.5 installed. But if not, then yeah it's 5.4 by default.

jpwhite4 · 2024-05-20T15:34:15Z

docs/upgrade.md

+Included in these new features is official support for the Rocky 8 operating system. This will allow organizations
+to migrate their XDMoD installations from the soon-to-be end-of-life CentOS 7 to a currently supported OS. The officially
+recommended process of migrating from a CentOS 7 XDMoD 10.0.X installation to an Rocky 8 XDMoD 10.5 installation is as follows:
+1. Install a fresh copy of XDMoD 10.5 on a new Rocky 8 server.


I think these instructions should say:

if you have Centos 7 10.0 XDMoD then you need to update to Centos 7 10.5 XDMoD. Then once you have Centos 7 10.5 then you dump and restore to Rocky 8 10.5. Then you update XDMoD 10.5 to 11.0

You can't take a dump of the 10.0 database from Centos7 and expect it to work on Rocky 8

Most definitely. Yeah, I should have updated the 10.0.x installation to be 10.5.0 and had another full entry for 10.0.x

I've updated this in the latest commits, let me know if what I've done works.

jpwhite4 · 2024-05-20T15:35:00Z

docs/upgrade.md

+### Server: EL8, XDMoD: 10.5, PHP: 7.2
+The steps to accommodate this scenario are fairly straightforward:
+
+*NOTE: There may be some PHP Warning messages generated during this process, they can be safely ignored. Once the


You need to say what PHP warning are generated.

and when to expect to see such warning. - saying that all the warnings can be deregarded is likely to cause us problems when someone has a fatal error that they ignore

I've updated this w/ the specific warnings to look out for in the latest commit

docs/upgrade.md

jpwhite4 · 2024-05-20T15:42:59Z

docs/upgrade.md

@@ -103,7 +155,7 @@ the recommended values listed in the [Configuration Guide][mysql-config].
 Open XDMoD 11.0.0 is a major release that includes new features along with many
 enhancements and bug fixes.

-Open XDMoD is now no longer bundled with any non-commercial licenses. The charting library used in Open XDMoD has changed from [Highcharts](https://www.highcharts.com/) to [Plotly JS](https://plotly.com/javascript/), an open source library. This transition removes the non-commercial license required from the Highcharts library. Please refer to the [license notices](notices.md) for more information about the open source licenses bundled with Open XDMoD. For more information please refer to [release notes](https://github.com/ubccr/xdmod/releases) for Open XDMoD 11.0.
+Open XDMoD is now no longer bundled with any non-commercial licenses. The charting library used in Open XDMoD has changed from [Highcharts](https://www.highcharts.com/) to [Plotly JS](https://plotly.com/javascript/), an open source library. This transition removes the non-commercial license required from the Highcharts library. Please refer to the [license notices](notices.md) for more information about the open source licenses bundled with Open XDMoD. For more information please refer to [release notes](https://github.com/ubccr/xdmod/releases) for Open XDMoD 11.0.


Open XDMoD is now no longer bundled with any non-commercial licenses.

To be fair Open XDMoD has never been bundled with any licenses ever. It has, however, been bundled with software libraries that have licenses that have additional restrictions for commercial and governmental use.

I certainly don't remember adding this bit, It sounds like something that would have come from the Plotly merge but I can certainly update it if we want to keep it in.

jpwhite4 · 2024-05-20T15:44:30Z

tests/artifacts/xdmod/regression/current/expected/reference/raw-data/jobs.json

Please can you confirm this is correct? Seems odd to be adding an empty file here

I would agree, I think this must have been generated while I was testing early on in development. I'll go ahead and remove this file.

This has been removed.

jpwhite4 · 2024-05-20T15:46:31Z

tests/ci/bootstrap.sh

+    mysql_install_db --user mysql
+
+    if [ -f /etc/my.cnf.d/mariadb-server.cnf.rpmsave ]; then
+        mv /etc/my.cnf.d/mariadb-server.cnf.rpmsave /etc/my.cnf.d/mariadb-server.cnf


This line seems unnecessary since the file is going to be overwritten 3 lines down on line 70.

Yup, it's removed in the latest

jpwhite4 · 2024-05-20T15:47:05Z

tests/ci/bootstrap.sh

+        mv /etc/my.cnf.d/mariadb-server.cnf.rpmsave /etc/my.cnf.d/mariadb-server.cnf
+    fi
+    if [ -f /etc/my.cnf.d/mariadb-server.cnf ]; then
+        >/etc/my.cnf.d/mariadb-server.cnf


Why test for the file to exist before overwriting it? Under what conditions will the file not exist?

no idea, testing has been removed in the lates

jpwhite4 · 2024-05-20T15:47:28Z

tests/ci/bootstrap.sh

+              datadir=/var/lib/mysql
+              socket=/var/lib/mysql/mysql.sock
+              log-error=/var/log/mariadb/mariadb.log
+              pid-file=/run/mariadb/mariadb.pid" > /etc/my.cnf.d/mariadb-server.cnf


see line 70

Yup, it's been updated in the latest.

In the interest of keeping things the same I'm adding the composer dep that will provide the PHPUnit `assertArraySubset` functionality that we were using prior to it being removed.

- Removed the SimpleSaml assertion install because this is now being handled in `build.json` - Removed the extraneous `|| true` from the mongodb step as it no longer returns a non-zero exit code. - Added `php-devel` to the PHP pre-reqs because these steps should be the same as what we detail in `upgrade.md` and users may not have `php-devel` installed.

Update per code review re using '*'.

This has been moved to `build.json`

I guess we do need the `|| true`.

.circleci/config.yml

Co-authored-by: Joe White <jpwhite4@buffalo.edu>

jpwhite4 · 2024-06-10T15:21:53Z

classes/ETL/DbModel/Entity.php

@@ -267,10 +267,14 @@ public function setSystemQuoteChar($char)

    public function quote($identifier)
    {
-        // Don't quote the identifier if it's already been quoted
+        // We can't quote non-strings so....
+        if (!is_string($identifier)) {


Is this the safest way to handle this case? What scenarios exist where the quote function was called with a non string object? What was the object and what happened next?

It's probably not, I've updated this in the latest commit to throw an Exception.

So just some background, this code works fine in 7.2|7.4, but it was failing in 8.0 due to 8.X being more stringent about types.

Making it explicit what happens when an attempt to quote a non-string identifier is made. In PHP 7.4 this does not trigger, in PHP 8.* it will ( or at least it did ).

ryanrath force-pushed the xdmod11-php8 branch from 64f3a3a to ad594c7 Compare February 2, 2024 20:33

ryanrath force-pushed the xdmod11-php8 branch from ad594c7 to aa045f5 Compare February 16, 2024 19:45

ryanrath added this to the 11.0.0 milestone Apr 30, 2024

ryanrath added Category:Infrastructure Internal infrastructure updates/changes dependencies Pull requests that update a dependency file labels Apr 30, 2024

ryanrath force-pushed the xdmod11-php8 branch 2 times, most recently from bae4fb8 to 58129c1 Compare April 30, 2024 15:00

ryanrath marked this pull request as ready for review May 1, 2024 15:09

ryanrath force-pushed the xdmod11-php8 branch from 3d77611 to 117af77 Compare May 7, 2024 18:16

jpwhite4 requested changes May 10, 2024

View reviewed changes

classes/DB/PDODBMultiIngestor.php Show resolved Hide resolved

.circleci/config.yml Show resolved Hide resolved

.circleci/config.yml Outdated Show resolved Hide resolved

tests/ci/samlSetup.sh Outdated Show resolved Hide resolved

ryanrath force-pushed the xdmod11-php8 branch 2 times, most recently from 0a89ec0 to 1e63e8e Compare May 16, 2024 15:24

ryanrath requested a review from jpwhite4 May 16, 2024 15:26

ryanrath force-pushed the xdmod11-php8 branch from 1e63e8e to 525f278 Compare May 16, 2024 16:05

ryanrath requested review from eiffel777, aaronweeden, connersaeli and aestoltm May 16, 2024 17:13

ryanrath force-pushed the xdmod11-php8 branch 2 times, most recently from b0e55d2 to e101fbf Compare May 17, 2024 19:00

jpwhite4 reviewed May 20, 2024

View reviewed changes

docs/upgrade.md Outdated Show resolved Hide resolved

jpwhite4 reviewed May 20, 2024

View reviewed changes

spark0r added 21 commits June 7, 2024 11:43

Updates per code review

e996e98

forgot to return values

68fbbe5

appeasing the style gods

cebf3a0

Adding phpunitarraysubset back

6f37189

In the interest of keeping things the same I'm adding the composer dep that will provide the PHPUnit `assertArraySubset` functionality that we were using prior to it being removed.

appeasing the style gods... again

b9937c4

Pinning phpmailer/phpmailer to a version

8a15723

Update per code review re using '*'.

Removing extraneous code

02c9784

This has been moved to `build.json`

Upgrade doc updates per code review

61fa9d9

Turns out we do need this

ba55069

I guess we do need the `|| true`.

Just some minor wordsmithing

779593b

additional wordsmithing

2f10c4a

lksdjf

973db40

more appeasing

5b9b72d

testing php 7.4

ec2d5db

missed

4410b2f

Missed this some how

8276c78

addressing PHP Deprecation notice

ca0c621

updating mentions of PHP8 to PHP7.4

b0200be

trying to get rid of deprecation notice

48d21d4

alksjdf

988a69f

ryanrath force-pushed the xdmod11-php8 branch from 195f150 to 988a69f Compare June 7, 2024 15:43

jpwhite4 reviewed Jun 7, 2024

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

Update .circleci/config.yml

d0fd8c4

Co-authored-by: Joe White <jpwhite4@buffalo.edu>

ryanrath requested a review from jpwhite4 June 10, 2024 13:56

Being more specific on phpword version

491410a

jpwhite4 requested changes Jun 10, 2024

View reviewed changes

Throwing exception as opposed to swallowing error case

c65cc9e

Making it explicit what happens when an attempt to quote a non-string identifier is made. In PHP 7.4 this does not trigger, in PHP 8.* it will ( or at least it did ).

jpwhite4 approved these changes Jun 10, 2024

View reviewed changes

ryanrath merged commit 1ca059d into ubccr:xdmod11.0 Jun 11, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XDMoD 11.0 PHP 7.4 Changes #1806

XDMoD 11.0 PHP 7.4 Changes #1806

ryanrath commented Jan 8, 2024 •

edited

Loading

jpwhite4 May 20, 2024

ryanrath May 20, 2024

jpwhite4 May 20, 2024

ryanrath May 20, 2024

jpwhite4 May 20, 2024

ryanrath May 20, 2024

ryanrath May 21, 2024

jpwhite4 May 20, 2024

jpwhite4 May 20, 2024

ryanrath May 20, 2024

jpwhite4 May 20, 2024

ryanrath May 20, 2024

jpwhite4 May 20, 2024

ryanrath May 20, 2024

ryanrath May 21, 2024

jpwhite4 May 20, 2024

ryanrath May 20, 2024

jpwhite4 May 20, 2024

jpwhite4 May 20, 2024

ryanrath May 20, 2024

jpwhite4 May 20, 2024

ryanrath May 20, 2024

jpwhite4 Jun 10, 2024

ryanrath Jun 10, 2024

ryanrath Jun 10, 2024

XDMoD 11.0 PHP 7.4 Changes #1806

XDMoD 11.0 PHP 7.4 Changes #1806

Conversation

ryanrath commented Jan 8, 2024 • edited Loading

Description

Motivation and Context

Tests performed

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanrath commented Jan 8, 2024 •

edited

Loading