-
Notifications
You must be signed in to change notification settings - Fork 19
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 URL length calculations for long Permalinks #288
Fix URL length calculations for long Permalinks #288
Conversation
…permalink length. Define the "transformed" URL length, according to the Twitter docs: https://developer.twitter.com/en/docs/counting-characters. Added "twitterURLLength" to the adminAutoshareForTwitter localized object. Refactored classic editor and block editor codebases to prefer the "twitterURLLength" value, if available.
b8d29e7
to
084a63a
Compare
@justinmaurerdotdev Thank you for the PR and for sharing useful information on how Twitter counts URL characters. I will get this reviewed very soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@justinmaurerdotdev Thank you for the PR and very detailed information. This is amazing. We may need to change a bit on how we want to keep using the current URL length counter for local setups instead of identifying using fallback based on the value of AUTOSHARE_FOR_TWITTER_URL_LENGTH
constant. Otherwise, All looks good to me. I have added some comments on the code to check.
Thank you.
…because Twitter doesn't shorten inaccessible links
I've refactored this a bit to have the |
Sorry for the extra notifications today. Was able to test in both types of environments and realized the JS was sometimes doing string concatenation instead of numeric addition. Otherwise, I'm pretty sure this is (at least functionality-wise) ready to go. I'm not sure what it means that I'm assigned on this PR, but let me know if you need anything from me. |
…) issue in classic editor length calculation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @justinmaurerdotdev, Sorry for the delay in the review. Thanks for making the additional changes. PR looks amazing now. I have made minor tweaks to the usage of fixed URL length and additional slash(/) in URL length calculation in the classic editor. PR is ready to merge. Please let me know if you see anything wrong with the tweaks I made, I would love to fix it.
Thanks again for this great contribution.
Was out of the office last week, but this looks good to me. Very glad to see this fixed. Thanks for working with me on it! |
Description of the Change
There were a couple of issues present having to do with the calculation of tweet length.
First, the calculation for the Classic Editor was using the permalink preview to calculate the hypothetical permalink length. That's great until the permalink gets long, and then the preview is shortened with "...". So, the real permalink would be longer than the calculated one.
Second, Twitter currently shortens all URLs to 23 characters. So, technically, any URL should just be calculated as 23 characters.
From https://developer.twitter.com/en/docs/counting-characters
From https://developer.twitter.com/en/docs/tco, evidently there's an API endpoint that returns the current maximum URL length. In some cases, shorter URLs may not reach the maximum length, but in all cases, the maximum is the maximum.
In this PR, I:
AUTOSHARE_FOR_TWITTER_URL_LENGTH
, to store the current URL length published by Twitter. This could be set to false to revert to using the real URL lengths. This could be refactored to get the most current value from thehelp/configuration
endpoint, as described here.localize_data()
asadminAutoshareForTwitter.twitterURLLength
.localize_data()
implementation to force real permalink calculations (because each implementation falls back to real permalink calculation when thetwitterURLLength
is false.compose_tweet_body()
method to also use the defined twitter URL length for its calculations.How to test the Change
To be honest, I am not familiar with the Cypress framework enough to write an automated test. And I'm not sure if all of the files I've touched are in scope for those types of tests anyway. I would be happy to help pair-program tests for this, or dialogue about them here.
To manually test the correct functionality, there are two scenarios to test: 1) the correct default, which is to treat all URLs as the same length and 2) the fallback, in case we want to disable that feature.
To test the fallback
AUTOSHARE_FOR_TWITTER_URL_LENGTH
value tofalse
in the main plugin file.To test the currently-intended behavior
AUTOSHARE_FOR_TWITTER_URL_LENGTH
value back to23
in the main plugin file.This should work in Classic Editor and Gutenberg (I think, but wasn't sure about the plugin build process). In both cases, the Tweet should successfully send without errors when the post is published.
Changelog Entry
Credits
Props @justinmaurerdotdev
Checklist:
I get
Error: ENOENT: no such file or directory, scandir '/Users/justinmaurer/.wp-env'
When I try to run thecypress:run
script. I'm not really familiar with.wp-env
and don't know what's expected to be in there (or how to set it up).I'm aware there is probably some discussion or cleanup needed to make my commits fit the package standards, but I wanted to submit it at this stage to get some feedback about what exactly is missing, and whether there's some other plan (or better way) to handle this.