Shortcodes: enable inline PDFs

[lancewillett]

Fixes #14959

Changes proposed in this Pull Request:

Adds new file to enable inline PDF embed handling.

The block editor view:

Before, frontend:

After, frontend:

Is this a new feature or does it add/remove features to an existing part of Jetpack?

• New feature. Internal reference: p7DVsv-8fu-p2

Testing instructions:

• Edit a post or page
• Put a PDF file URL on its own line (not a link, just bare URL)
• Preview the post
• You should see a visual embed instead of the bare URL

If not supported in a given environment, the embed object should fall back to a plain link.

Proposed changelog entry for your changes:

• Enable inline PDF embeds

Apologies if I missed a lot of steps any important code flags and formatting... Jetpack development has increased in complexity 10x since I last contributed a patch.

[jetpackbot]

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: April 7, 2020.
Scheduled code freeze: March 31, 2020

Generated by 🚫 dangerJS against fc617a9

[jeherve]

When testing this, what was the steps you followed to get a plain link in the editor? When I first pasted the link into the editor, the embed failed and I got this:

I then had to convert to link, and then remove the link.

That makes me wonder if this shouldn't be better handled in a new block. In fact, the Gutenberg team seems to be considering adding its own PDF block:
WordPress/gutenberg#19077

---

Could you fix all PHPCS warnings for that file? Our pre-commit hook would not let you commit this otherwise:

yarn php:lint modules/shortcodes/inline-pdfs.php                                
yarn run v1.21.1
$ composer php:lint modules/shortcodes/inline-pdfs.php
> vendor/bin/phpcs -p -s 'modules/shortcodes/inline-pdfs.php'
E 1 / 1 (100%)



FILE: ...ev/wp-content/plugins/jetpack/modules/shortcodes/inline-pdfs.php
----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------
 1 | ERROR | There must be no blank lines before the file comment
   |       | (Squiz.Commenting.FileComment.SpacingAfterOpen)
 6 | ERROR | There must be exactly one blank line after the file
   |       | comment
   |       | (Squiz.Commenting.FileComment.SpacingAfterComment)
 6 | ERROR | Missing @package tag in file comment
   |       | (Squiz.Commenting.FileComment.MissingPackageTag)
 9 | ERROR | Missing doc comment for function
   |       | inline_pdf_embed_handler()
   |       | (Squiz.Commenting.FunctionComment.Missing)
----------------------------------------------------------------------

---

Since this is a copy of a WordPress.com feature, I think it would be useful to synchronize the 2 features between the 2 codebases. You can see r191075-wpcom for an example of the file we update to keep those things in sync.

---

This may be an opportunity to have a wider release of the feature on WordPress.com? Do you know if there is a specific reason why the feature is still restricted to a12s there?

Edit: I found the related thread at p7DVsv-8fu-p2. I update the PR description accordingly.

---

Here is a quick diff to address some of the feedback I left below:

diff --git a/modules/shortcodes/inline-pdfs.php b/modules/shortcodes/inline-pdfs.php
index 41d76396d..b780082b6 100644
--- a/modules/shortcodes/inline-pdfs.php
+++ b/modules/shortcodes/inline-pdfs.php
@@ -1,16 +1,38 @@
 <?php
-
 /**
  * Inline PDFs
- * Takes one-line embeds of a PDF URL (*.pdf), and attempts to embed it directly in the post instead of leaving it as a link.
+ *
+ * Takes one-line embeds of a PDF URL (*.pdf),
+ * and attempts to embed it directly in the post instead of leaving it as a link.
+ *
+ * @package Jetpack
+ */
+
+wp_embed_register_handler( 'inline-pdfs', '#https?://[^<]*\.pdf$#i', 'jetpack_inline_pdf_embed_handler' );
+
+/**
+ * Callback to modify output of inline PDF URLs.
+ *
+ * @param array $matches Regex partial matches against the URL passed.
+ * @param array $attr    Attributes received in embed response.
+ * @param array $url     Requested URL to be embedded.
  */
-wp_embed_register_handler( 'inline-pdfs', '#https?://[^<]*\.pdf$#i', 'inline_pdf_embed_handler' );
+function jetpack_inline_pdf_embed_handler( $matches, $attr, $url ) {
+	/** This action is documented in modules/widgets/social-media-icons.php */
+	do_action( 'jetpack_bump_stats_extras', 'embeds', 'pdf' );
 
-function inline_pdf_embed_handler( $matches, $attr, $url ) {
-	return sprintf(
-		'<object data="%1$s" type="application/pdf" width="100%%" height="800">
-		  <p><a href="%1$s">%1$s</a></p>
-		</object>',
-		esc_attr( $url )
-	);
+	if ( class_exists( 'Jetpack_AMP_Support' ) && Jetpack_AMP_Support::is_amp_request() ) {
+		return sprintf(
+			'<p><a href="%1$s">%2$s</a></p>',
+			esc_url( $url ),
+			esc_html__( 'PDF Document', 'jetpack' )
+		);
+	} else {
+		return sprintf(
+			'<object data="%1$s" type="application/pdf" width="100%%" height="800">
+				<p><a href="%1$s">%1$s</a></p>
+			</object>',
+			esc_attr( $url )
+		);
+	}
 }

[lancewillett]

Thank you, @jeherve! That's a massive boost to my contribution. I'll make the suggested changes, keep testing, and ping back for another review.

That makes me wonder if this shouldn't be better handled in a new block

I'd like to suggest we push this now, separately — as it'll still serve a purpose for many people. Just a plain URL embed, easy.

[simison]

This seems pretty major change to how people's content might change...

Should you have this as a setting and default off to old site, or not do this for posts before feature release date?

[lancewillett]

@simison Thank you for the important note on how front-end content appearance might change when this ships and sites are updated.

I considered the behavior change and opted to proceed with the improvement for several reasons.

1. It is a much better user experience.
2. Authors who've used PDF links are unlikely to use a bare URL on one line — they'd typically wrap in an anchor element.
3. The PDF viewer is native to browsers, robust, and adaptive to devices.
4. We'll closely watch the adoption and feedback on WordPress.com to inform the Atomic and Jetpack site impact.
5. We can use the Jetpack release notes, blog post, and other announcement channels to showcase the improvement as it is much better (see point 1).
6. I believe that it is a fairly quick fix to adapt content (see point 2).

[lancewillett]

Hi @jeherve All feedback address except for the AMP support. Choosing to not add that for now.

✅ Unit test added, passes
✅ Fixed prefix and stats bump
✅ Lint errors fixed (except for "missing doc comment" in unit test)
✅ Tested the behavior — it only works if a PDF is on its own line and not wrapped in an anchor element (bare URL)

[lancewillett]

This file should be synced with WordPress.com. How do we accomplish that?

[jeherve]

This works well in my tests. Merging.

This file should be synced with WordPress.com. How do we accomplish that?

I'll take care of this in D41074-code.

[jeherve]

r205041-wpcom