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

Update Travis CI config, fix broken tests, update README, and diverse minor fixes #1503

Merged
merged 15 commits into from
Jul 26, 2018

Conversation

benlk
Copy link
Collaborator

@benlk benlk commented Jul 25, 2018

Changes

  • Implements the matrix-based Travis CI config originally written for NPR's Story API plugin, as the research conducted in that issue's comments is still valid regarding which versions of PHP and PHPunit are supported by WordPress.
    • We can support PHP 7.2 in tests once https://core.trac.wordpress.org/ticket/43218 is merged; this is likely to happen with WordPress 5.0
    • Drops support for PHP 5.3 and 5.5, gaining support for PHP 5.6
    • Gains support for PHP 5.6, 7.0, 7.1
  • Removes a portion of the Travis CI config that would have messaged our Hipchat room; we stopped using Hipchat a while ago.
  • Changes PHP support to whatever versions of PHP are currently receiving updates
  • Changes WordPress support to a recommendation to use the most-recent version, with a pointer to .travis.yml to read the list of PHP versions we test against.
  • As collateral damage of updating the supported PHP/Wordpress versions:
    • Updates the staff list in README.md
    • Updates the version number of the current Largo release in README.md
  • Adds checks in largo_post_social_links() to make sure that the array keys it's trying to access exist before accessing them.
  • Changes of_sanitize_multicheck() to initialize a variable accessed as an array with an array instead of an empty string.
  • Changes largo_remove_hero() to fix Failing test PostTemplatesTestFunctions::test_insert_image_no_thumb #1404:
    • adjusts the regular expression pattern to no longer require the src="" attribute, as it's not needed
    • `trim()s the post content to remove empty leading newlines after removing an empty first paragraph tag caused by removing the duplicate hero image.
    • updates tests and test documentation for this function, and adds tests for a couple more conditions.

Why

Two-part:

@benlk benlk added this to the 0.6 - Performance & SEO milestone Jul 25, 2018
@benlk benlk self-assigned this Jul 25, 2018
@benlk benlk requested a review from tylermachado July 25, 2018 18:25
@benlk
Copy link
Collaborator Author

benlk commented Jul 25, 2018

Failed tests:

largo_fb_user_is_followable used the Facebook Follow Button,
https://developers.facebook.com/docs/archive/docs/plugins/follow-button/
in order to determine whether a user or page was followable. By using
this public button's HTML markup, we were able to make the followability check
without using the Facebook Graph API.

Facebook deprecated that button on February 5, 2018.

To query the Graph API, we need to have an API key.

To require Largo users to acquire an API key, for the sole purpose of checking
whether their Facebook page is followable, seems excessive.

Therefore, since there's no reasonable way to check whether a Facebook
profile is followable, deprecate the functionality by making the
function always return false, and clean out its tests. If WP_DEBUG or LARGO_DEBUG
are set, provide a deprecation notice to the error log.
@benlk
Copy link
Collaborator Author

benlk commented Jul 25, 2018

The Facebook Followable issue is because Facebook deprecated the Follow button on Feb 5, 2018: https://developers.facebook.com/docs/archive/docs/plugins/follow-button/

So I've added a commit that deprecates largo_fb_user_is_followable and removes it from all places in largo that aren't tests or necessary for legacy support.

@benlk
Copy link
Collaborator Author

benlk commented Jul 25, 2018

1) PostTemplatesTestFunctions::test_insert_image_no_thumb
C1
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'<h2>Headings</h2>
+'<p></p>
+<h2>Headings</h2>
 <p>Integer posuere erat a ante venenatis dapibus posuere velit aliquet. Sed posuere consectetur est at lobortis. Nulla vitae elit libero, a pharetra augue. Fusce dapibus, tellus ac cursus commodo, tortor mauris condimentum nibh, ut fermentum massa justo sit amet risus. Donec sed odio dui.</p>'

So the change in 75aad91 successfully removes the image, but doesn't get an opening paragraph tag.

The relevant return statement is return str_replace( $matches[0], '', $content );.

There are a few simple fixes here.

  • return str_replace( '<p>' . $matches[0] . '</p>', '', $content );, which assumes that the image is wrapped in an empty p tag. It doesn't remove the image if the p tag isn't empty. It doesn't remove the image if the p tag contains classes or other attributes.
  • after doing the str_replace, remove the first instance of /<p[^>]?><\/p>/ so long as it's the first line in the post. We can only remove the first one iff it's the first line, because sometimes wpautop adds more <p></p> in the post. The [^>]? wildcard is to catch paragraph classnames.

For #1503 (comment)

This change:

- removes an empty <p></p>
- removes an empty <p class="whatever"></p>
- removes the image but not the p if it's a <p><img/>foo</p>
- checks that the foo is left behind
- checks that a non-matching image isn't removed
…urrent post does not have featured media thumbnail
@benlk
Copy link
Collaborator Author

benlk commented Jul 25, 2018

There was 1 error:
1) FeaturedMediaTestAjaxFunctions::test_largo_save_featured_image_display
An unexpected error occurred. Something may be wrong with WordPress.org or this server&#8217;s configuration. If you continue to have problems, please try the <a href="https://wordpress.org/support/">support forums</a>. (WordPress could not establish a secure connection to WordPress.org. Please contact your server administrator.)
/tmp/wordpress/src/wp-includes/update.php:306
/tmp/wordpress/src/wp-includes/update.php:631
/tmp/wordpress/src/wp-includes/plugin.php:525
/tmp/wordpress/tests/phpunit/includes/testcase-ajax.php:193
/tmp/wordpress/src/wp-content/themes/largo/tests/inc/test-featured-media.php:187

Well ... that's an error.

But it's not an error I'm worried about.

@benlk
Copy link
Collaborator Author

benlk commented Jul 25, 2018

We're down to this in PHP 7.1:

1) UpdateTestFunctions::test_largo_perform_update
Illegal string offset 'facebook'
/tmp/wordpress/src/wp-content/themes/largo/lib/options-framework/options-sanitize.php:47
/tmp/wordpress/src/wp-includes/class-wp-hook.php:298
/tmp/wordpress/src/wp-includes/plugin.php:203
/tmp/wordpress/src/wp-content/themes/largo/lib/options-framework/options-framework.php:363
/tmp/wordpress/src/wp-content/themes/largo/inc/update.php:551
/tmp/wordpress/src/wp-content/themes/largo/inc/update.php:73
/tmp/wordpress/src/wp-content/themes/largo/tests/inc/test-update.php:68

@benlk
Copy link
Collaborator Author

benlk commented Jul 26, 2018

whoa it worked

@benlk benlk changed the title Update Travis CI config, update README, and diverse other minor fixes Update Travis CI config, fix broken tests, update README, and diverse minor fixes Jul 26, 2018
@benlk benlk mentioned this pull request Jul 26, 2018
Copy link

@tylermachado tylermachado left a comment

Choose a reason for hiding this comment

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

Everything looks good to me.

@benlk benlk merged commit 1c1023e into 0.5-dev Jul 26, 2018
@benlk benlk deleted the 1404-fix-failing-test branch July 26, 2018 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failing test PostTemplatesTestFunctions::test_insert_image_no_thumb
2 participants