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

Conditionally set auto_detect_line_endings when lower than PHP 8.1 #2540

Closed
wants to merge 5 commits into from

Conversation

EvanHerman
Copy link
Contributor

@EvanHerman EvanHerman commented Jul 27, 2023

Description

Conditionally set auto_detect_line_endings in the ical parser when on PHP < 8.1.

Since PHP 8.1, PHP INI directive auto_detect_line_endings is deprecated. Setting it to true emits a PHP deprecation notice. This INI directive will be removed in PHP 9.0.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • PHP 8 compatibility

Checklist:

  • My code is tested

@EvanHerman EvanHerman added the [Type] Enhancement Something new that adds functionality label Jul 27, 2023
@EvanHerman EvanHerman requested a review from olafleur July 27, 2023 15:03
@EvanHerman EvanHerman self-assigned this Jul 27, 2023
@EvanHerman EvanHerman changed the title Conditionally set auto_detect_line_endings when lower than PHP 8 Conditionally set auto_detect_line_endings when lower than PHP 8.1 Jul 27, 2023
@EvanHerman EvanHerman requested review from AnthonyLedesma and removed request for olafleur August 7, 2023 17:55
@AnthonyLedesma AnthonyLedesma added this to the 3.1.0 milestone Aug 7, 2023
@AnthonyLedesma
Copy link
Member

Kinda, related here is that it seems like the Events block ical parser is completely broken now with PHP 7.4. I think we should disable the feature when running PHP 7.4 at this point. We cannot even download and run 7.4 due to security/support issues now. Is still around one some hosts but we should still work toward non-support.

Copy link
Member

@AnthonyLedesma AnthonyLedesma left a comment

Choose a reason for hiding this comment

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

Damn. It looks like the Events block ICS parser has been silently failing for a while. This is confirmed not to be a failure with 6.3. I get a visual error when running locally with SCRIPT_DEBUG and WP_DEBUG running PHP 8.0.28 with wp-env.

Our E2E test is giving a false positive because it is checking for an error state, but the ICS parser giving a silent failure. It seems to properly parse the objects, however, that is not happening in reality.

Silent fail in prod, and apparently in some CI instances.
Screenshot 2023-08-08 at 5 33 56 AM

Fail seen in 7.4, and running 8.0 locally with wp-env.
Screenshot 2023-08-08 at 5 35 05 AM

api-fetch.js?ver=c6922e5e289e31508e9e:727     GET http://localhost:8888/index.php?rest_route=%2Fwp%2Fv2%2Fblock-renderer%2Fcoblocks%2Fevents&context=edit&attributes%5BexternalCalendarUrl%5D=https%3A%2F%2Fcalendar.google.com%2Fcalendar%2Fical%2F8hohgb8qv19fgvjbbkcehe0ce0%2540group.calendar.google.com%2Fpublic%2Fbasic.ics&attributes%5BeventsRange%5D=all&attributes%5BeventsToShow%5D=5&attributes%5BshowCarousel%5D=true&post_id=4&_locale=user 500 (Internal Server Error)

@AnthonyLedesma
Copy link
Member

I realize now, that I forgot about this bug! What a bummer. Let's hold off on making any more changes to the events block for now I think.

@AnthonyLedesma AnthonyLedesma deleted the ical-parser-php8 branch August 8, 2023 12:50
@AnthonyLedesma AnthonyLedesma removed this from the 3.1.0 milestone Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement Something new that adds functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants