Problem/Motivation

PHP 8.2 is now supported by D7.94, views, file_entity, i18n, entity_translation, media_youtube, media, the date module, entity_translation_unified_form and many other contrib modules. ctools being the most important module other than views and a dependency of views we need to push forward with my proposed patch number 16 which is passing all tests and is ready to be committed/reviewed by a maintainer.

Related cicd issues were resolved today, lets go!

Steps to reproduce

Run automated tests and see.

Proposed resolution

Write a patch, see related issues and how it was done (easy)

Remaining tasks

Review and commit patches

API changes

TBD

CommentFileSizeAuthor
#3 ctools-php82x-3327350-09.patch379 bytesjoseph.olstad
#6 ctools_sept_7_2022.png23.09 KBjoseph.olstad
#21 no_change.patch204 bytesjoseph.olstad
#16 ctools-php82x-3327350-16.patch722 bytesjoseph.olstad
#41 3327350-41.patch884 bytesjoelpittet

Comments

joseph.olstad created an issue. See original summary.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented 
exception: [Deprecated function] Line 153 of sites/all/modules/ctools/plugins/export_ui/ctools_export_ui.class.php:
Creation of dynamic property views_ui::$items is deprecated

Easy fix, just add this one line above the class ctools_export_ui class declaration

#[\AllowDynamicProperties]
class ctools...// Like this ^^^

see related issues for how it's done if you're curious.

#3327343: Fully Support PHP 8.2

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
StatusFileSize
new ctools-php82x-3327350-09.patch379 bytes
joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Status: Active » Needs review
joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Issue tags: +PHP 8.2
joseph.olstad’s picture

joseph.olstad
Primary language French
commented
StatusFileSize
new ctools_sept_7_2022.png23.09 KB

why? ctools code did not change between Sept 7 and Sept 14, however the ci-cd setup likely did.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

***EDIT***Issue caused by an incorrect version restriction in the new contrib 'color' project, this was a core module since 2006 and was only recently added to contrib, intended for D9.4.x+/D10+ . The version contraints are not registered correctly and for some reason the cicd tries to bring in "color" from contrib when it should only use the D7 core color module.
***END EDIT***

likely that cicd is using an incorrect version of composer for D7. Weird how I've noticed only an issue with ctools so far, maybe there's other contrib modules with the same issue.

could be due to ctools 8.x being the default branch instead of 7.x

for ex, the views module default branch is 7.x-3.x because D10 views is included in core, views does NOT have this issue.

file_entity does NOT have this issue, has both 8x and 7x branches however the 7.x-2.x branch is the default branch.

media does NOT have this issue, again, default branch is 7.x-2.x not 8x

i18n does NOT have this issue, does NOT have an 8x/9x/10x branch

seems to me like a cicd issue

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Issue summary: View changes
joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Issue summary: View changes
joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Title: Fully Support PHP 8.2 » Fully Support PHP 8.2, see parent issue
Parent issue: » #3332391: The contrib color project is conflicting with the D7 core color project, breaking ctools 7.x tests and possibly others
joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Issue summary: View changes
poker10’s picture

poker10 commented
Related issues: +#3332436: Refactor & correct list of removed core modules

I have discussed this composer issue on Slack with @drumm and others and it seems like this needs to be fixed in the project_composer, see: #3332436: Refactor & correct list of removed core modules.

Not sure if it would be possible to trick composer to use the color module from core by using minimum version number in the .info file (as @mglaman suggested, something like this drupal:color (>=7.40)), but if not, we need to wait until that is resolved.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Status: Needs review » Reviewed & tested by the community

RTBC patch #9

We need this change for PHP 8.2 compatibility, it's the same change that's been used in Drupal 10 core, in other drupal 7 contrib modules such as webform and i18n and others.

The test fails are unrelated and this change will not cause a regression.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

#3335458: D7 ctools head tests affected by D10 deprecations - See upstream issues

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Parent issue: #3332391: The contrib color project is conflicting with the D7 core color project, breaking ctools 7.x tests and possibly others » #3332436: Refactor & correct list of removed core modules
Related issues: -#3332436: Refactor & correct list of removed core modules +#3332391: The contrib color project is conflicting with the D7 core color project, breaking ctools 7.x tests and possibly others
joseph.olstad’s picture

joseph.olstad
Primary language French
commented
StatusFileSize
new ctools-php82x-3327350-16.patch722 bytes

Ok new patch

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Issue summary: View changes
joseph.olstad’s picture

joseph.olstad
Primary language French
commented

This fix is ready for maintainers eyes.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Reviewed & tested by the community » Needs review

Need a second pair of eyes from the community to RTBC.
There are no failing tests that this patch resolves (that I could see).
No steps to reproduce the errors though it's marked as a feature request the title is nebulous about the testbot composer issues.
This needs a better title and a clear set of instructions to test so we know the changes are not just for change sake.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

@joseph.olstad Sorry not meant to derail your progress. Thanks for taking the time to get things ready for PHP 8.2 and fixing the color issue that has plagued me in other issues too!

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
StatusFileSize
new no_change.patch204 bytes

To reproduce the errors, run the automated tests against HEAD with PHP 8.2

Here's a no_change patch to prove failures.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Title: Fully Support PHP 8.2, see parent issue » Support PHP 8.2 with ctools
Category: Feature request » Bug report
joseph.olstad’s picture

joseph.olstad
Primary language French
commented

The above no_change.patch fails PHP 8.2 testing.

In order to resolve the ctools PHP 8.2 compatibility issues please review/apply/commit/tag/release ctools with patch 16 . Patch 16 resolves 4 failures. If you turn on PHP 8.2 testing here; https://www.drupal.org/node/343333/qa you'll notice this.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

@Joel Pittet, to give you a bit more background information, I've written PHP 8.2 compatibility patches for the following modules and they have been committed and released:

entity_translation
#3327347-37: Fully Support PHP 8.2
file_entity
#3327347: Fully Support PHP 8.2
date module
#3328051: PHP 8.2 - Fix deprecated dynamic properties
media
#3332117: Media - PHP 8.2 compatibility
i18n
#3327343: Fully Support PHP 8.2
entityreference
#3334678: PHP 8.2 compatibility fix
webform
#3333988: PHP 8.2 compatibility fix

and a few others.

These are all very similar fixes to patch 16.

This is a very simple fix that we need to move forward on in order to keep fixing the modules that depend on ctools. Please consider fast tracking patch 16. There's a lot of other projects I'd like to continue fixing and this one is holding things up.

i18n, entityreference, webform, entity_translation, file_entity, media, media_youtube, the date module, these all have recently had PHP 8.2 compatible tagged releases. Let's please keep the momentum moving forward because PHP 8.2 is very very fast and it's well worth the trouble!

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

@joel pittet, in PHP 8.2 Dynamic Properties are deprecated in the sense that they are now disabled by default on all classes. This is to improve performance and reduce memory consumption. If a class doesn't need dynamic properties support PHP 8.2 does not load it unless explicitly asked on a class by class basis.

Patch 16 above is very simple, adding two missing properties to one class and explicitly enabling Dynamic properties on the other.

The upgrade to PHP 8.2 is a significant performance improvement and once again lowers memory footprint.

Patch 16 resolves 4 HEAD failures as illustrated above. Patch 16 is very similar to the rest of the PHP 8.2 fixes that have already gone into core and many contrib modules and dependencies.

The webform module needed a fix due to someone misspelling a property , previous versions of php just added the property dynamically but PHP 8.2 will not do this unless the class has a Dynamic Properties directive.

I hope that we can fast track this fix, if you have any other concerns related to patch 16 please raise them and we can discuss. From my perspective this is ready and will not cause regressions with previous releases. We need the ctools fixes because the ctools module is used by so many other modules.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Fixed

Oh the illustration with the failed tests really helps and the detailed explaination, thanks again @joseph.olstad

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

I committed this to D9 and wonder if it's relevant (will test tomorrow) but since you are doing some 8.2 support I'll ask your take?

#3336655: PHP 8.2: ${var} in strings is deprecated

D7 findings

❯ ag '\$\{'
includes/export.inc
283:    if (empty(${$export['identifier']})) {
291:    $item = ${$export['identifier']};
1237:    $output .= "  \${$export['identifier']}s = array();\n\n";
1240:      $output .= "  \${$export['identifier']}s['" . check_plain($object->{$export['key']}) . "'] = \${$export['identifier']};\n\n";
1242:    $output .= "  return \${$export['identifier']}s;\n";

page_manager/page_manager.module
1202:      $code .= "  \${$export['identifier']}s['" . check_plain($object->{$export['key']}) . "'] = \${$export['identifier']};\n\n";
1204:    $code .= "  return \${$export['identifier']}s;\n";
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Fixed » Needs work

I noticed a few more while enabling a bunch ctools modules:

Deprecated function: Creation of dynamic property view::$export_module is deprecated in _ctools_export_get_defaults() (line 689 of /var/www/html/public/sites/all/modules/contrib/ctools/includes/export.inc).
Deprecated function: Creation of dynamic property view::$export_type is deprecated in ctools_export_load_object() (line 538 of /var/www/html/public/sites/all/modules/contrib/ctools/includes/export.inc).
Deprecated function: Creation of dynamic property view::$in_code_only is deprecated in ctools_export_load_object() (line 539 of /var/www/html/public/sites/all/modules/contrib/ctools/includes/export.inc).
Deprecated function: Creation of dynamic property view::$table is deprecated in ctools_export_load_object() (line 540 of /var/www/html/public/sites/all/modules/contrib/ctools/includes/export.inc).
Deprecated function: Creation of dynamic property view::$type is deprecated in ctools_export_load_object() (line 537 of /var/www/html/public/sites/all/modules/contrib/ctools/includes/export.inc).
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Fixed

I just realized #29 is all the view class from views which has been fixed already in dev with #[\AllowDynamicProperties]

#28 I'm still investigating but we can create a follow-up

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Status: Fixed » Active

Just going to trigger a no change test because ci on main project returns error today

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Status: Active » Fixed

Just looking at comment 21 no change patch, expecting pass on all tests

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

Ok 21 is passing, @Joel Pittet could you please retrigger the project tests for ctools? Why would the no change patch work and the project tests on commit , not work?
Perhaps retrigger those and we can review that. Required maintainer prerms to do this

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

I'll retrigger them thanks. I don't think the stuff I mentioned in #28 above have test coverage.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue tags: +ContributioneWeekend2023
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue tags: -ContributioneWeekend2023 +ContributionWeekend2023
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Deprecated function: Creation of dynamic property ctools_context::$arg_length is deprecated in ctools_plugin_example_context_create_simplecontext() (line 80 of /var/www/html/public/sites/all/modules/contrib/ctools/ctools_plugin_example/plugins/contexts/simplecontext.inc).

One more

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Fixed » Needs work

Maybe ctools_context should also have #[\AllowDynamicProperties], that might help other ecosystem modules that add stuff to context?

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

And same for ctools_context_required

Deprecated function: Creation of dynamic property ctools_context_required::$restrictions is deprecated in ctools_context_required->__construct() (line 275 of /var/www/html/public/sites/all/modules/contrib/ctools/includes/context.inc).
Deprecated function: Creation of dynamic property ctools_context_required::$restrictions is deprecated in DrupalDatabaseCache->prepareItem() (line 449 of /var/www/html/public/includes/cache.inc).

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Needs review
StatusFileSize
new 3327350-41.patch884 bytes

I did some testing and exporting and #28 is not an issue.

Thoughts on this patch in relation to the comments #38 and #40?

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Parent issue: #3332436: Refactor & correct list of removed core modules » #3195523: Plan for CTools 7.x-1.21 release
Related issues: +#3332436: Refactor & correct list of removed core modules

Changing parent to the release plan #3195523: Plan for CTools 7.x-1.21 release

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

Patch looks good, put it in as a compatibility insurance policy IMHO good to go in.

I want to see a tagged release asap and this looks like a step in the right direction. We need a ctools tagged release so that views automated tests take it. My reason for doing this is for views as views requires ctools.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Fixed

I'll release this soon, just need to get the branch to pass the tests

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

weird that the above no change patch keeps working but the actual ctools project tests are outputing such a strange output about being hundred+ commits behind and what's this about a production branch?

https://dispatcher.drupalci.org/job/drupal_d7/246546/console

I've added a comment here:

#3332436-13: Refactor & correct list of removed core modules

Maybe try removing the broken ci tests from the project and re-add them?

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

I asked around and hestenet mentioned this was the issue #3315509: phpcs endless loop with "Drupal" rules due to missing @endcode so I did those fixes here #3337800: phpcs endless loop with "Drupal" rules due to missing @endcode

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

@joelpittet, and @hestenet , thanks for that!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.