Problem/Motivation

PHP 8.4 has deprecated implicit nullable types.

Note that...

  1. Elasticsearch Connector says the minimum version of Drupal core that it supports is 9.2.
  2. Drupal 9.2 says the minimum version of PHP that it supports is 7.3.0.
  3. Nullable types were introduced in PHP 7.1.0.
  4. There are other instances of nullable type declarations in the module already.

Steps to reproduce

Use in a PHP 8.4 environment

Proposed resolution

Use explicit nullable types (compatible with PHP 7.1+ so no compatibility concerns given Drupal core PHP minimums).

This is a backwards-compatibility break, but this branch is still in alpha, and thus, backwards-compatibility breaks are still allowed.

Remaining tasks

  1. Write a merge request - !82 created by @beloglazov91 in #2
  2. Review and feedback - done by @mparker17 in #6
  3. RTBC and feedback - done by @mparker17 in #6
  4. Commit - done by @mparker17 in #7
  5. Release - released in 8.x-7.0-alpha7

User interface changes

None.

API changes

The function signature of the public function \Drupal\elasticsearch_connector\Plugin\search_api\backend\SearchApiElasticsearchBackend::deleteItems has changed from deleteItems(IndexInterface, array):mixed to deleteItems(?IndexInterface, array):mixed. This is a backwards-compatibility break, but this branch is still in alpha, and thus, backwards-compatibility breaks are still allowed.

Data model changes

None.

CommentFileSizeAuthor
#5 3506266-5-elasticsearch_connector-7.x-nullable-types-explicit.patch652 bytessprouse_moose

Issue fork elasticsearch_connector-3506266

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

beloglazov91 created an issue. See original summary.

beloglazov91 opened merge request !82

mparker17 made their first commit to this issue’s fork.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

I've rebased this merge request onto the latest changes to 8.x-7.x

sprouse_moose’s picture

sprouse_moose commented
StatusFileSize
new 3506266-5-elasticsearch_connector-7.x-nullable-types-explicit.patch652 bytes

The rebased changes don't yet exist in the latest 8.x-7.x release. The MR has not yet been merged.
Adding a patch to resolve issue in the interim.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

@sprouse_moose, thank you; and sorry for the churn.

I wanted to get the most-breaking-changes out in their own release, so that the module can finally become maintainable again (this release of most-breaking-changes became 8.x-7.0-alpha6).

While I'd love to merge everything that I can right away, I also need to give the community a bit of time to adjust to the changes... besides patches in the public queue, many people maintain internal patches as well, and they need time to adjust those. 8.x-7.0-alpha6 in particular was A LOT of work, because I wanted to make it as easy as possible for people to upgrade after all the breaking changes, so I fixed merge conflicts in ~57 issues on my own time (just fixing the merge requests took me more than 20 hours of work, excluding meals, breaks, and much-needed sleep).

Anyway, I plan to make another release in ~2 weeks time with bugfixes and new features.

If you want to help, you could test out the change that has already been merged in #3183164: Index-time boost is deprecated .

If you're using the change in this issue, it is actually pretty helpful for maintainers to know that! We're aware that any change that we make to the module could break ~4000 sites using this module at time-of-writing, which is a lot of pressure! When people report that they've reviewed the change, and/or they're using it and it hasn't broken their site, it gives us more confidence to merge it without worrying that all 4000 people are going to start yelling at us! 😅

Taking a look at the changes in this merge request...

  1. Elasticsearch Connector says the minimum version of Drupal core that it supports is 9.2.
  2. Drupal 9.2 says the minimum version of PHP that it supports is 7.3.0.
  3. Nullable types were introduced in PHP 7.1.0.
  4. There are other instances of nullable type declarations in the module already.
  5. I don't see any other instances of type declarations that could be nullable but are not yet marked as such after this change.
  6. It's not feasible to write automated tests for this change.

I've updated the issue summary with this information (this helps maintainers make a decision about whether it's safe to merge).

The code in this issue fixed a phpstan lint that had previously been ignored. I've deleted the override in phpstan-baseline.neon. And, testbot is now green across the board.

Testing myself locally, I can confirm that it works, so I'm marking this as RTBC.

I will merge this change shortly.

Thank you everyone!

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

I've merged this change and credited both @beloglazov91 and @sprouse_moose.

(I assumed that, because @sprouse_moose needed the patch, they had reviewed the changes and were successfully using it on their site)

Thank you everyone!

I will update this issue when the change gets released.

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes

This has been released in 8.x-7.0-alpha7!

Status: Fixed » Closed (fixed)

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