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

Empty checker Validator #71

Merged
merged 41 commits into from
May 12, 2016
Merged

Empty checker Validator #71

merged 41 commits into from
May 12, 2016

Conversation

everton-rosario
Copy link
Contributor

@everton-rosario everton-rosario commented Apr 20, 2016

This validator checks for all Element's required fields. If any element is invalid isValid(), it will generate a empty content on the toDOMElement() method.

  • All Elements will have a method isValid()
    toDOMElements will have an if (!$this->isValid()) -> Wont output itself.
  • Transformer will checks all isValid() from elements, if not valid, will add a warning to the transformation

This way we dont:

  • Output invalid/empty elements
  • Fail silently (because we added warnings to the Transformer process)

// Stripes empty spaces, &nbsp;, <br/>, new lines
$text = strip_tags($text);
$text = preg_replace("/[\r\n\s]+/", "", $text);
$text = str_replace("&nbsp;", "", $text);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure where the $text param comes from, but for me in my attempts to strip whitespace for checking if a DOMElement::nodeValue was empty, I needed to do:

$trimmed = trim($child->nodeValue, " \t\n\r\0\x0B\xC2\xA0");
if (!empty($trimmed)) {
  return FALSE;
}

@m4olivei
Copy link
Contributor

m4olivei commented May 4, 2016

@everton-rosario are you finished with updates here? If so I'll try to find time to take it for a spin on our site.

@everton-rosario
Copy link
Contributor Author

@m4olivei Im almost done on this new feature.
Im now creating more test cases to cover it better.
No new feature will be implemented, its just a matter of improving tests and finding bugs.

if ($this->credits) {
if (is_array($this->credits)) {
foreach ($this->credits as $paragraph) {
if (Type::is($paragraph, Element::getClassName())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noted that on:

https://github.com/facebook/facebook-instant-articles-sdk-php/blob/ed25a2209ff2019fec92d417566c06fc5125b7d5/src/Facebook/InstantArticles/Elements/Footer.php#L68

We are not enforcing the type of the array to be only Paragraphs. Please add this validation there.

Copy link
Contributor Author

@everton-rosario everton-rosario May 11, 2016

Choose a reason for hiding this comment

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

It can be String or string[] as well. This would be a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

*/
public function isValid()
{
return true;
Copy link
Contributor

@diegoquinteiro diegoquinteiro May 11, 2016

Choose a reason for hiding this comment

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

Consider returning invalid if it contains any textChildren (as it'll not be rendered).

@everton-rosario everton-rosario merged commit 9a9a8b8 into master May 12, 2016
@everton-rosario everton-rosario deleted the empty_checker branch May 12, 2016 19:51
@ghost ghost added the CLA Signed label Jul 12, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants