Widgets: Porting of WP.com's Flickr Widget by Copons · Pull Request #6942 · Automattic/jetpack

Copons · 2017-04-06T19:08:12Z

This is my second PR in the effort of filling the gaps between Jetpack and WP.com, to smooth the AT process out.

Changes proposed in this Pull Request:

Adds the Flickr widget as already available on WP.com.

Testing instructions:

Add the widget to a sidebar and check if the widget shows up.
Check if the photos are retrieved from the correct RSS feed.

jeherve

This looks good. I just made a few comments about small changes.

jeherve · 2017-04-07T19:51:32Z

modules/widgets/flickr/widget.php

+								<img
+									alt="<?php echo $photo['title']; ?>"
+									border="0"
+									src="<?php echo $photo['src']; ?>"


I'm thinking we could apply Photon to those images if the module is enabled, like we do for the Image and the Text widget.

Looking into it!

jeherve · 2017-04-07T19:52:08Z

modules/widgets/flickr/widget.php

+						<?php foreach ( $photos as $key => $photo ) { ?>
+							<a href="<?php echo $photo['link']; ?>">
+								<img
+									alt="<?php echo $photo['title']; ?>"


Should we esc_attr here, just in case?

jeherve · 2017-04-07T19:52:23Z

modules/widgets/flickr/widget.php

+				<tr>
+					<td align="center">
+						<?php foreach ( $photos as $key => $photo ) { ?>
+							<a href="<?php echo $photo['link']; ?>">


Should we escape here too?

jeherve · 2017-04-07T19:52:30Z

modules/widgets/flickr/widget.php

+									alt="<?php echo $photo['title']; ?>"
+									border="0"
+									src="<?php echo $photo['src']; ?>"
+									title="<?php echo $photo['title']; ?>"


Should we escape here too?

jeherve · 2017-04-07T19:52:35Z

modules/widgets/flickr/widget.php

+								/>
+							</a><br /><br />
+						<?php } ?>
+						<a href="<?php echo $flickr_home; ?>">


Should we escape here too?

jeherve · 2017-04-07T19:53:33Z

modules/widgets/flickr/widget.php

@@ -0,0 +1,33 @@
+<!-- Start of Flickr Widget -->
+<table


Why the table layout? Would it be possible to use regular divs, so folks can customize the layout with a bit less trouble?

Yeah, and it has been quite cringeworthy to write! 😄

Fact is: that's the wpcom version code.
The point of this porting is so that wpcom sites will look and behave the same after an AT.
And so, what if someone customized the CSS of their Flickr widget and changing the HTML or the class names will screw it up?

eliorivero

In addition to the comment in the code:

After adding this, I get these in front end:

Notice: rss.php is <strong>deprecated</strong> since version 3.0.0! Use wp-includes/class-simplepie.php instead. in /wp-includes/functions.php on line 3958

Notice: Trying to get property of non-object in /wp-content/plugins/jetpack/modules/widgets/flickr.php on line 84

Notice: Undefined variable: flickr_home in /wp-content/plugins/jetpack/modules/widgets/flickr/widget.php on line 24

eliorivero · 2017-04-07T20:45:52Z

modules/widgets/flickr/form.php

+
+<p>
+	<small>*
+		<?php esc_html_e( 'To find your Flickr RSS feed, go to your photostream and click the "Edit" tab at the top. Scroll down to the bottom of the page until you see the RSS icon or the Latest link. Right click on either and copy the URL. Paste into the box above.', 'jetpack' ); ?>


a) This should be updated since there are no tabs now. There's a More link that unfolds a menu. Edit is now there.

b) Would be great if the text could be trimmed down to a point where it fits nicely below the input field, even in more than one line.

Copons · 2017-04-08T00:26:23Z

@jeherve All the $photo properties and $flickr_home are escaped during the $photos array creation:

jetpack/modules/widgets/flickr.php

Lines 94 to 100 in 32b20bb

    
           	array_push( $photos, array( 
        
           		'href'  => esc_url( $photochannel['link'], array( 'http', 'https' ) ), 
        
           		'src'   => esc_url( $src, array( 'http', 'https' ) ), 
        
           		'title' => esc_attr( $photo['title'] ), 
        
           	) ); 
        
           } 
        
           $flickr_home = esc_url( $rss->channel['link'], array( 'http', 'https' ) );

Copons · 2017-04-08T00:36:27Z

@eliorivero I was sure I protected those variables in case of undefined, but I might be confusing it with another PR. :/
Gonna see to replace the RSS library and try a leaner explanation text.

jeherve · 2017-04-10T14:03:09Z

All the $photo properties and $flickr_home are escaped during the $photos array creation:

jetpack/modules/widgets/flickr.php

Lines 94 to 100 in 32b20bb

array_push( $photos, array(

'href' => esc_url( $photochannel['link'], array( 'http', 'https' ) ),

'src' => esc_url( $src, array( 'http', 'https' ) ),

'title' => esc_attr( $photo['title'] ),

) );

}

$flickr_home = esc_url( $rss->channel['link'], array( 'http', 'https' ) );

Yeah, I noticed that and I think that's fine, but I would still escape them right before they get rendered, as outlined here:
https://vip.wordpress.com/documentation/vip/best-practices/security/validating-sanitizing-escaping/#always-escape-late

sirbrillig

Works for me!

dereksmart

works well for me

… of feed which is not in the UI for this widget

eliorivero · 2017-04-24T21:54:12Z

Works fine. Made a small change to the wording to reference always the same input label instead of "feed" which is not found in the UI for this widget. It's good to go and will merge once Travis ends.
I also rebased this branch to update it after the Cookie Law widget was merged.

* Changelog: initial commit for 4.9 release. * Changelog: add #6929 * Changelog: move old changelogs to changelog.txt * Readme: restore deleted release post link. The post is now live. * Changelog: add #6853 * Changelog: add #6856 * Changelog: add #6857 * Changelog: add #6884 * Changelog: add #6885 * Changelog: add #6892 * Changelog: add #6894 * Changelog: add #6898 * Changelog: add #6899 * Changelog: add #6900 * Changelog: add #6909 * Changelog: add #6927 * Changelog: add #6947 * Chagelog: add #6958 * Changelog: add #6961 * Changelog: add #6963 * Changelog: add #6965 * Changelog: add #6986 * Changelog: add #7000 * Changelog: add #7013 * Changelog: add #7015 * Changelog: add #7019 * Changelog: add #7028 * Changelog: add #6998 * Changelog: add #6999 * Changelog: add #7044 * Changelog: add #6881 * Changelog: add #6922 * Changelog: add #6940 * Changelog: add #6962 * Changelog: add #6942 * Changelog: add #6959 * Changelog: add #7018 * Changelog: add #6948 * Changelog: add #6657 * Changelog: add #7030 * Changelog: add #7048 * Changelog: add #7031 * Changelog: add #6990 * Changelog: add #6957 * Changelog: add #7027

Copons added [Feature] Extra Sidebar Widgets [Status] In Progress New Feature Task [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Apr 6, 2017

jeherve requested changes Apr 7, 2017

View reviewed changes

jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. [Pri] Normal and removed [Status] Needs Review This PR is ready for review. labels Apr 7, 2017

eliorivero suggested changes Apr 7, 2017

View reviewed changes

Copons added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Apr 10, 2017

beaulebens changed the title ~~Widgets: Porting of WP.com's Flickr~~ Widgets: Porting of WP.com's Flickr Widget Apr 10, 2017

sirbrillig self-requested a review April 20, 2017 21:44

sirbrillig approved these changes Apr 20, 2017

View reviewed changes

jeherve added [Pri] High and removed [Pri] Normal labels Apr 21, 2017

jeherve modified the milestone: 4.9 Apr 24, 2017

dereksmart approved these changes Apr 24, 2017

View reviewed changes

dereksmart 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 Apr 24, 2017

Copons added 3 commits April 24, 2017 18:43

Initial commit

d38afae

Add front end widget

43ea833

Add update function

4d4b820

eliorivero force-pushed the add/widgets/flickr branch from 2b346e3 to 5d5b55c Compare April 24, 2017 21:45

Copons and others added 8 commits April 24, 2017 18:45

Add style

0f1ca3d

Fix update function declared outside class

7090d06

Remove excessive styles

d16897b

Replace rss.php with SimplePie

daa0eca

Escape late the feed variables

6194494

Update and move the Flickr RSS URL description under its input field

658dba0

Apply Photon to the images

5d5b55c

Widgets - Flickr: update wording to reference the field label instead…

97fdc1c

… of feed which is not in the UI for this widget

eliorivero approved these changes Apr 24, 2017

View reviewed changes

eliorivero merged commit c420b2c into master Apr 24, 2017

eliorivero deleted the add/widgets/flickr branch April 24, 2017 22:18

eliorivero removed the [Status] Ready to Merge Go ahead, you can push that green button! label Apr 24, 2017

jeherve added the [Status] Has Changelog label Apr 24, 2017

jeherve added a commit that referenced this pull request Apr 24, 2017

Changelog: add #6942

faeb560

Conversation

Copons commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Testing instructions:

Uh oh!

jeherve left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eliorivero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copons commented Apr 8, 2017

Uh oh!

Copons commented Apr 8, 2017

Uh oh!

jeherve commented Apr 10, 2017

Uh oh!

sirbrillig left a comment

Choose a reason for hiding this comment

Uh oh!

dereksmart left a comment

Choose a reason for hiding this comment

Uh oh!

eliorivero commented Apr 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Copons commented Apr 6, 2017 •

edited

Loading

eliorivero commented Apr 24, 2017 •

edited

Loading