nuvoleweb / ui_patterns

[NOTE] Development has moved to https://drupal.org/project/ui_patterns
https://drupal.org/project/ui_patterns
GNU General Public License v2.0
85 stars 56 forks source link

Issue #342 by Grimreaper: PHP 8.1 compatibility. First try. #343

Closed FlorentTorregrosa closed 2 years ago

FlorentTorregrosa commented 2 years ago

Fix #342

heddn commented 2 years ago

:+1, LGTM

kvantstudio commented 2 years ago

Works for me.

erik-seifert commented 2 years ago

This is the last blocking for update our projects to php 8.1 and drupal 9.4. Please go on ;- )

steveoriol commented 2 years ago

With ui_patterns 8.x-1.2 and D9.4.1 + php8.1.7 I get this error: Deprecated function: Return type of Drupal\ui_patterns\Definition\PatternDefinition::offsetExists($offset) should either be compatible with ArrayAccess::offsetExists(mixed $offset): bool, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in include() (line 13 of modules/contrib/ui_patterns/src/Definition/PatternDefinition.php).

ademarco commented 2 years ago

I've no particular opinion on this, @FlorentTorregrosa please proceed as it makes the most sense, taking into account the community's feedback.

FlorentTorregrosa commented 2 years ago

@steveoriol Have you applied the patch from the PR? Without changes from this PR I got the same error. After applying the changes, no more error.

FlorentTorregrosa commented 2 years ago

@ademarco I can't merge because "At least 1 approving review is required by reviewers with write access."

Can you please approve the changes and/or give @pdureau write access? Or do you want to proceed in another way?

steveoriol commented 2 years ago

@FlorentTorregrosa, Yes no more error with the PR. Merci.

ademarco commented 2 years ago

I've merged GitHub actions, we should rebase this PR on current main branch, so we trigger the CI. We should also add PHP 8.1 to the CI itself, here:

        PHP_VERSION: ["7.4", "8.0"]
philippemellenger commented 2 years ago

The patch works great for me, thanks :+1:

FlorentTorregrosa commented 2 years ago

Hi,

PR rebased.

I have added PHP 8.1 in the PHP Version matrix, but it seems that tests use the ci.yml file from the main branch and not the one from the PR.

So I pushed my branch on this repository directly: https://github.com/nuvoleweb/ui_patterns/tree/342-php81

Waiting for results.

FlorentTorregrosa commented 2 years ago

Seems that tests are ok on PHP 8.1 :).

@ademarco Is it ok for you to merge?

FlorentTorregrosa commented 2 years ago

ui_patterns-8.x-1.2-php81-compatibility.txt

Patch to be able to apply on 8.x-1.2

claudiu-cristea commented 2 years ago

@FlorentTorregrosa could you, please, do a release with this change as ensures PHP 8.1 compatibility. I think it's worth it

FlorentTorregrosa commented 2 years ago

@claudiu-cristea Ok for sure.

I was about to create one when:

@ademarco can you please create a new release with updated CHANGELOG.md? Otherwise I can create the tag and releases on both Github and Drupal.org, just the file will not be up-to-date. But as the end goal is to move back to drupal.org, is it important?

ademarco commented 2 years ago

@FlorentTorregrosa you can create a release branch with only the changelog, then merge that one, and release from there.

claudiu-cristea commented 2 years ago

@FlorentTorregrosa Now you have #363. Would that be enough?

FlorentTorregrosa commented 2 years ago

@claudiu-cristea Thanks, from what I see this is almost the same from what I have done locally (and forgot to push...).

Would you like to join the Drupal Slack channel https://drupal.slack.com/archives/C03MFN4D40J ?

Also please see https://www.drupal.org/project/ui_suite/issues/3309394. I will do the release next week at DrupalCon, and try to migrate on Drupal.org.

claudiu-cristea commented 2 years ago

@FlorentTorregrosa thank you, it would be good to have it as soon as possible

mika2na commented 1 year ago

With 8.x-1.3 version released it should be ok.