Skip to content

Conversation

@kraftbj
Copy link
Contributor

@kraftbj kraftbj commented Apr 25, 2019

functions.photon.php contains a number of PHPCS issues and use of outdated methods (parse_url with a wrapper to incompletely do what wp_parse_url does now, seeding a rand, etc).

The major change is to the way we determine the subdomain. The issue with the previous way, beyond the PHPCS violation, is that srand is disabled on some systems. The benefit of seeding with the image URL is to net a consistent subdomain to avoid the same image being served from two different subdomains, thus having a browser download the same image twice.

Since we don't really need anything particular randomized, we can convert the image path to a value between 0 and 2 using a combo of abs, crc32 and the % operator.

Changes proposed in this Pull Request:

  • PHPCS fix and modernize Photon functions.

Testing instructions:

  • WIth Jetpack and Image CDN/Photon, verify Photon URL is the same as before.

Proposed changelog entry for your changes:

  • n/a, part of a general line item wrt PHPCS repair.

@kraftbj kraftbj added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins [Status] Needs Review This PR is ready for review. labels Apr 25, 2019
@kraftbj kraftbj added this to the 7.3 milestone Apr 25, 2019
@kraftbj kraftbj requested a review from a team April 25, 2019 17:14
@jetpackbot
Copy link
Collaborator

jetpackbot commented Apr 25, 2019

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: November 5, 2019.
Scheduled code freeze: October 29, 2019

Generated by 🚫 dangerJS against 4f59b6c

@kraftbj kraftbj added [Status] In Progress and removed [Status] Needs Review This PR is ready for review. labels Apr 25, 2019
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello kraftbj! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D27441-code before merging this PR. Thank you!

@kraftbj
Copy link
Contributor Author

kraftbj commented Apr 29, 2019

Moving this out of milestone so I can look at the wp.com diff in more detail before merging here.

@kraftbj kraftbj removed this from the 7.3 milestone Apr 29, 2019
@stale
Copy link

stale bot commented Jul 28, 2019

This PR has been marked as stale. This happened because:

  • It has been inactive in the past 3 months.
  • It hasn’t been labeled `[Pri] Blocker`, `[Pri] High`.

No further action is needed. But it's worth checking if this PR has clear testing instructions, is it up to date with master, and it is still valid. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

@matticbot
Copy link
Contributor

kraftbj, Your synced wpcom patch D27441-code has been updated.

@kraftbj
Copy link
Contributor Author

kraftbj commented Oct 10, 2019

Fusion trips up a bit when there are changes at the beginning of a file (e.g. new file-level docblock) when the corresponding wpcom file has the "this is in Jetpack" notice. Manually patched in D27441-code.

@matticbot
Copy link
Contributor

kraftbj, Your synced wpcom patch D27441-code has been updated.

@kraftbj kraftbj force-pushed the photon/use_wp_parse_url branch from 59fb72e to 4f59b6c Compare October 24, 2019 19:47
@kraftbj kraftbj added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Oct 24, 2019
@kraftbj kraftbj added this to the 7.9 milestone Oct 24, 2019
@kraftbj
Copy link
Contributor Author

kraftbj commented Oct 24, 2019

The only functional change is the subdomain determination. Per the advice of Systems ( pMz3w-arH-p2 ), in 7.9, I'd like to only use the new technique for 1/3rd of URLs and then roll out to another third in 8.0, etc.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Oct 25, 2019
@jeherve jeherve merged commit fb414bd into master Oct 25, 2019
@jeherve jeherve deleted the photon/use_wp_parse_url branch October 25, 2019 07:41
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins [Status] Tested on WP.com Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants