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

Exceptions thrown when using PHP 7.2 #2948

Closed
hosef opened this issue Jan 20, 2018 · 8 comments
Closed

Exceptions thrown when using PHP 7.2 #2948

hosef opened this issue Jan 20, 2018 · 8 comments

Comments

@hosef
Copy link

hosef commented Jan 20, 2018

In PHP 7.2, each() has been marked as deprecated and count() throws an exception when used on non countable types. These 2 behaviour changes cause a lot of errors in backdrop, and should be fixed.


PR by @hosef: backdrop/backdrop#2159

@herbdool
Copy link

This sounds useful @hosef. I see you closed your PR--are you going to create an updated one?

@hosef
Copy link
Author

hosef commented Mar 26, 2018

I made the original pull request from my main branch, which blocked me from working on other issues.
I will make a new branch and re-post the changes some time today.

@drupol
Copy link

drupol commented Apr 16, 2018

Hi @hosef ,

Did you made a new PR ?

Thanks!

@hosef
Copy link
Author

hosef commented Apr 30, 2018

I was finally able to get back to this and I have a new pull request available based on the changes from https://www.drupal.org/project/drupal/issues/2947772.

I did not apply the Changes to Tar.php from Drupal because the Backdrop version did not have any of the errors updating. There were also several code differences between Drupal and Backdrop's versions, so I was not sure how to apply the changes.

@quicksketch
Copy link
Member

Thanks @hosef! I read over the PR at backdrop/backdrop#2159 and it looks great. All changes make sense to me, though both the old and new syntaxes are a bit brain-melting. It's with good reason why such changes were made in PHP 7.2.

There were also several code differences between Drupal and Backdrop's versions, so I was not sure how to apply the changes.

I only found one missing change (other than the Tar classes) and that was in LocaleLanguageNegotiationInfoFunctionalTest. In Backdrop this was fixed in Issue #175: Simplifying config translation storage. So no change is needed there.

In Backdrop the updater and installer use Zip files (as that's what's hosted on GitHub) rather than Tar files, so we're not affected in the same way. However we do still use .tar files when downloading config exports.

@hosef Could you see if you can create a problem when visiting /admin/config/development/configuration/full/export and clicking the button there to export a full config archive in PHP 7.2? Likewise there may be problem when importing the config archives.

@quicksketch quicksketch added this to the 1.9.6 milestone Apr 30, 2018
@hosef
Copy link
Author

hosef commented Apr 30, 2018

I tested the config export and it did have some errors, so I have applied the changes from https://www.drupal.org/project/drupal/issues/2946045 to system.tar.inc, and that allowed me to do a config export/import without errors.

@quicksketch
Copy link
Member

Yay, that's great. Honestly I think we should use zip files for the config import/export as well and stop using tar files entirely, but for the purposes of this issue, we're all good here! Thanks so much for your work in porting this patch. All changes look good to me and all tests are passing. I am fairly sure we have complete coverage of all changes from the D7 patch where they apply to Backdrop.

@quicksketch
Copy link
Member

I have merged backdrop/backdrop#2159 into 1.x and 1.9.x. Thanks @hosef!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants