Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

Fix "Empty string supplied as input" bug #647

Merged
merged 5 commits into from Jun 5, 2017
Merged

Conversation

abdusfauzi
Copy link
Contributor

@abdusfauzi abdusfauzi commented Mar 20, 2017

This PR:

Fixes #
Fix PHP Warning: DOMDocument::loadHTML(): Empty string supplied as input, by verifying the content and making sure if it is empty, do not transformString.

Automattic/facebook-instant-articles-wp#628

Fix `PHP Warning:  DOMDocument::loadHTML(): Empty string supplied as input`, by verifying the content and making sure if it is empty, do not `transformString`.
@abdusfauzi abdusfauzi changed the title Fix #628 - "Empty string supplied as input" bug Fix "Empty string supplied as input" bug Mar 20, 2017
Copy link
Collaborator

@everton-rosario everton-rosario left a comment

Choose a reason for hiding this comment

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

get_the_content calls has few filters, and probably calling twice should be avoided.

Thanks for your PRs @abdusfauzi

@@ -690,7 +690,9 @@ public function to_instant_article() {
Video::setDefaultCommentEnabled( $settings_publishing[ 'comments_on_media' ] );
}

$transformer->transformString( $this->instant_article, $this->get_the_content(), get_option( 'blog_charset' ) );
if ( '' != $this->get_the_content() ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this happening to you?
I'd change this, so we call only once the get_the_content() method here.
Can you do it like this:

$the_content = $this->get_the_content();
if ( '' != $the_content ) {
   $transformer->transformString...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. No problem 👌🏻

Copy link
Contributor Author

@abdusfauzi abdusfauzi left a comment

Choose a reason for hiding this comment

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

@everton-rosario Rewrite the fix to reduce the get_the_content() call only once.

@@ -690,7 +690,9 @@ public function to_instant_article() {
Video::setDefaultCommentEnabled( $settings_publishing[ 'comments_on_media' ] );
}

$transformer->transformString( $this->instant_article, $this->get_the_content(), get_option( 'blog_charset' ) );
if ( '' != $this->get_the_content() ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. No problem 👌🏻

$transformer->transformString( $this->instant_article, $this->get_the_content(), get_option( 'blog_charset' ) );

$the_content = $this->get_the_content();
if ( '' != $the_content ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please use the Type::isTextEmpty() method for this? Ty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vkama i'm sorry, i wish i understand how to use the suggested Type:isTextEmpty() 😰 Would you mind show me sample?

Copy link
Collaborator

@vkama vkama May 31, 2017

Choose a reason for hiding this comment

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

use Facebook\InstantArticles\Validators\Type;

if (!Type::isTextEmpty($the_content)) { ... }

@vkama vkama self-requested a review May 29, 2017 14:06
@vkama vkama changed the base branch from master to v4.0 May 31, 2017 13:25
@vkama vkama added this to the 4.0 milestone May 31, 2017
@@ -691,7 +693,7 @@ public function to_instant_article() {
}

$the_content = $this->get_the_content();
if ( '' != $the_content ) {
if (!Type::isTextEmpty($the_content)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

good job ty

@vkama vkama merged commit 212372f into Automattic:v4.0 Jun 5, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants