Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #3260861 by tbsiqueira, navneet0693, apaderno: Add support for php 8.0 #2961

Closed
wants to merge 17 commits into from

Conversation

tbsiqueira
Copy link
Contributor

@tbsiqueira tbsiqueira commented May 30, 2022

Problem

Add PHP 8 support to Open Social

Solution

Refactor code to support PHP 8 and maintain backward compatibility.

  1. Updated php configurations from 7.4 to 8.0 in travis.yml, and github actions.
  2. Temporarily changed drupal_social branch to 'dev-issue/3260861-php8-support'

Issue tracker

https://www.drupal.org/project/social/issues/3260861

Theme issue tracker

N/A

How to test

  • All checks should pass.
  • Open Social installation should be flawless on both PHP 7.4 and PHP 8.1 installations.
  • Smoke test to see nothing break.

Definition of done

Before merge

  • Code/peer review is completed
  • All commit messages are clear and clean. If applicable a rebase was performed
  • All automated tests are green
  • Functional/manual tests of the acceptance criteria are approved
  • All acceptance criteria were met
  • If applicable (I.E. new hook_updates) the update path from previous versions (major and minor versions) are tested
  • This pull request has all required labels (team/type/priority)
  • This pull request has a milestone
  • This pull request has an assignee (if applicable)
  • Any front end changes are tested on all major browsers
  • New UI elements, or changes on UI elements are approved by the design team
  • New features, or feature changes are approved by the product owner

After merge

  • Code is tested on all branches that it has been cherry-picked
  • Update hook number might need adjustment, make sure they have the correct order
  • The Drupal.org ticket(s) are updated according to this pull request status

Screenshots

N.A

Release notes

We are now supporting PHP 8.0

Change Record

N.A

Translations

N.A

@tbsiqueira tbsiqueira changed the title Issue #3260861 by tbsiqueira, navneet0693, apaderno: Add support for … Issue #3260861 by tbsiqueira, navneet0693, apaderno: Add support for php 8.0 May 30, 2022
@tbsiqueira tbsiqueira force-pushed the feature/3260861-php-8-support branch from 98fb495 to 1f82247 Compare May 31, 2022 14:44
// possible
$parents = $this->entityTypeManager->getStorage('taxonomy_term')->loadParents((int) $term->id());
// The number of parents determines the current depth.
$depth = count($parents);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Kingdutch Please give this one a look. ^

Copy link
Member

@Kingdutch Kingdutch Jun 3, 2022

Choose a reason for hiding this comment

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

You pointed to OverviewTerms.php in Drupal core as an example of where they use depth so we can learn from them to make PHPStan happy.

// Depth should be added by Drupal core and is used in various places. 
// However it's also not officially documented, so in case Drupal core has not 
// made it available yet we load the depth ourselves and cache it on the term 
// entity.
if (!isset($term->depth)) {
  $parents = $this->entityTypeManager->getStorage('taxonomy_term')->loadParents((int) $term->id());
  $term->depth = count($parents);
}
// To make PHPStan happy we assert that `depth` is now indeed numeric since the types defined for magic `__get` actually return `FIeldItemListInterface` which is not the case for our special `depth` property.
assert(is_numeric($term->depth), "Expected TermInterface item to have a numeric depth but the value was of incorrect type or unset.");

$choice = new \stdClass();
$choice->option = [$term->id() => str_repeat('-', $term->depth) . $this->entityRepository->getTranslationFromContext($term)->label()];

@navneet0693 navneet0693 added the needs work: commit cleanup This change requires a decision record in Slite label Jun 3, 2022
@navneet0693 navneet0693 requested a review from Kingdutch June 3, 2022 06:18
- name: Checkout code
uses: actions/checkout@v2

- name: Clear composer cache
Copy link
Contributor

Choose a reason for hiding this comment

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

are these required? seems like they are not doing anything really

Screenshot 2022-06-03 at 09 44 49

.github/workflows/codingStandards.yml Outdated Show resolved Hide resolved
.github/workflows/testing.yml Show resolved Hide resolved
@tbsiqueira tbsiqueira force-pushed the feature/3260861-php-8-support branch from 3d72470 to aec6750 Compare June 3, 2022 09:29
Ressinel and others added 5 commits June 3, 2022 11:53
The property 'depth' is adding to the term in TermStorage::loadTree(), this is why we can't load it with the usual methods. Typecasting directly for $term->depth is not solve this error. So, to fix it we need to determine term depth with another approach.
@tbsiqueira tbsiqueira force-pushed the feature/3260861-php-8-support branch from 29a34d8 to d87f13e Compare June 3, 2022 09:54
@tbsiqueira
Copy link
Contributor Author

Closing this PR due to rebase issues replaces by #2976

@tbsiqueira tbsiqueira closed this Jun 3, 2022
@tbsiqueira tbsiqueira deleted the feature/3260861-php-8-support branch June 3, 2022 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs work: commit cleanup This change requires a decision record in Slite prio: critical team: nucleus
Development

Successfully merging this pull request may close these issues.

5 participants