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

DS-745 Support PHP 8 #2454

Merged
merged 9 commits into from Mar 22, 2022
Merged

DS-745 Support PHP 8 #2454

merged 9 commits into from Mar 22, 2022

Conversation

remydenton
Copy link
Collaborator

Jira

https://pegadigitalit.atlassian.net/browse/DS-745

Summary

Removes dependency on two composer packages that are incompatible with PHP 8

Details

This PR removes the following two dependencies, which are incompatible with PHP 8 and abandoned:

  1. tooleks/php-avg-color-picker. The last commit to this repo was 5 years ago and it was only used in one optional place to provide a color while images are loading.
  2. fzaninotto/faker. A mostly useless shiny object library that also happens to be an abandoned project. We had a whole custom PHP class to override it to get it to work with our test snapshots, and we weren't using it in any recent examples. I removed a few examples that were preventing PL from building (there are some others in the archive directory that as-is I left because I felt it was a waste of time to update them-- in the off chance we ever need them again, we can replace the text with Lorem Ipsum at that time).

For faker, I also had to remove it as a sub dependency of our fork of basaltinc/twig-tools: boltdesignsystem/twig-tools#2. We should first merge that PR, then update this PR to use the master branch before merging it.

How to test

Confirm no regressions on this branch. Confirm that faker text has been replaced with hard-coded lorem ipsum. Confirm that any differences in the color of the placeholder for bolt images while they load is minor/unnoticeable.

@github-actions github-actions bot added the type: feature List this PR in the 'Features' section of the release notes. label Mar 18, 2022
@colbytcook colbytcook had a problem deploying to feature/DS-745-support-php-8--branch-preview March 18, 2022 23:09 Failure
@andrewmriv
Copy link
Collaborator

The changes looked good and made sense to me. Unfortunately, it looks like there is a build error.

Image color is now hard-coded.
@colbytcook colbytcook had a problem deploying to feature/DS-745-support-php-8--branch-preview March 21, 2022 18:38 Failure
It's not clear to me which faker usage instances caused problems during
build and which didn't.  PL builds normally locally with these changes
but not without them, so hopefully this is sufficient.
@colbytcook colbytcook requested a deployment to feature/DS-541-floating-action-buttons--f3e0f767--commit-preview March 21, 2022 19:50 In progress
@danielamorse
Copy link
Collaborator

@remydenton This looks good to me. Agree it's a good time to remove these libraries.

I noticed there are some instances of faker still in the docs-site/src/pages/pattern-lab/_patterns/999-archive/academy folder. If these are intentionally ignored that's totally fine. If not, you might want to update these before merging (optional).

@colbytcook colbytcook temporarily deployed to feature/DS-745-support-php-8--branch-preview March 22, 2022 18:14 Inactive
@colbytcook colbytcook requested a deployment to bugfix/typeahead-button-position--0ffe8fa4--commit-preview March 22, 2022 18:50 In progress
@remydenton
Copy link
Collaborator Author

Thanks for the reviews guys. @danielamorse, as discussed on Slack I went ahead and removed the remaining faker instances. I also merged the forked twig-tools PR and updated this one to use its master branch.

@remydenton remydenton merged commit fcfef78 into master Mar 22, 2022
@remydenton remydenton deleted the feature/DS-745-support-php-8 branch March 22, 2022 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature List this PR in the 'Features' section of the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants