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

fix: Prevent crash from invalid media URLs #6235

Conversation

wpmobilebot
Copy link
Collaborator

Related PRs

Description

This PR is generated by version-toolkit to downstream the changes for gutenberg submodule.

@dcalhoun dcalhoun self-assigned this Sep 26, 2023
@dcalhoun dcalhoun force-pushed the version-toolkit/gutenberg/fix/prevent-crash-from-invalid-media-urls branch from 27e4d6c to 2f33bcc Compare September 26, 2023 17:55
@fluiddot fluiddot self-requested a review September 27, 2023 10:02
Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

Approved via WordPress/gutenberg#54834.

@dcalhoun it would be great to include this change in the release notes. I understand the solved crash is a regression introduced in 1.104.0 via WordPress/gutenberg#54096, is this accurate? If so, we should consider incorporating the fix as a beta fix into WP-iOS version 23.3. WDYT?

@dcalhoun
Copy link
Member

@dcalhoun it would be great to include this change in the release notes.

I was relying upon the expectation that the release process would grab the change log entry. I will proactively add a release note in this PR.

I understand the solved crash is a regression introduced in 1.104.0 via WordPress/gutenberg#54096, is this accurate? If so, we should consider incorporating the fix as a beta fix into WP-iOS version 23.3. WDYT?

No, this crash is actually reproducible as far back as the 22.7 release, which is the oldest available in TestFlight. This appears to be a long-standing crash.

Given it is a long-standing crash, the urgency feels lower. However, one could argue the error prone nature of typing on mobile device keyboards is prone to errors and thus we should prioritize addressing this. That said, I wonder how often someone types a long URL as opposed to pasting it. The latter could still produce an invalid URL, but less likely.

WDYT?

@fluiddot
Copy link
Contributor

I was relying upon the expectation that the release process would grab the change log entry. I will proactively add a release note in this PR.

Ah, I thought we updated the release notes per PR. The release process implies updating the release notes, but AFIK we only replace the Unreleased section with the version being cut.

No, this crash is actually reproducible as far back as the 22.7 release, which is the oldest available in TestFlight. This appears to be a long-standing crash.

@dcalhoun Oh, sorry for the confusion. I thought that this was related to the recent changes introduced in WordPress/gutenberg#54096. If this can be reproduced in older versions, I agree the urgency of providing a crash fix in the current release is lower than I originally thought.

Given it is a long-standing crash, the urgency feels lower. However, one could argue the error prone nature of typing on mobile device keyboards is prone to errors and thus we should prioritize addressing this. That said, I wonder how often someone types a long URL as opposed to pasting it. The latter could still produce an invalid URL, but less likely.

WDYT?

I agree, the main reason for considering this a beta fix was merely the fact of addressing a regression. I checked the Sentry event related to the crash and only impacted three users, so I think it doesn't have a severity/impact to consider it as a beta fix to incorporate into 23.3.

@dcalhoun
Copy link
Member

I was relying upon the expectation that the release process would grab the change log entry. I will proactively add a release note in this PR.

Ah, I thought we updated the release notes per PR. The release process implies updating the release notes, but AFIK we only replace the Unreleased section with the version being cut.

You are correct. My action (or inaction) was a cryptic, un-communicated change in my behavior relating to Git flow and release processes discussions (p9ugOq-3yd-p2#comment-6868) that have not yet been affirmed across the wider team. It was ill-advised on my part. The team should fully aware and committed prior to altering process. Sorry about that.

@dcalhoun dcalhoun merged commit 3115977 into trunk Sep 27, 2023
14 of 15 checks passed
@dcalhoun dcalhoun deleted the version-toolkit/gutenberg/fix/prevent-crash-from-invalid-media-urls branch September 27, 2023 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants