Skip to content

Use length property instead of PHP≥7.2 count() method on DOMNodeList#3727

Merged
westonruter merged 1 commit intodevelopfrom
fix/domnodelist-count-usage
Nov 13, 2019
Merged

Use length property instead of PHP≥7.2 count() method on DOMNodeList#3727
westonruter merged 1 commit intodevelopfrom
fix/domnodelist-count-usage

Conversation

@westonruter
Copy link
Copy Markdown
Member

Summary

This fixes an issue reported in the support forums: https://wordpress.org/support/topic/error-1275/:

Fatal error: Uncaught Error: Call to undefined method DOMNodeList::count() in .../includes/sanitizers/class-amp-core-theme-sanitizer.php:1597 Stack trace: 
#0 .../includes/sanitizers/class-amp-core-theme-sanitizer.php(522): AMP_Core_Theme_Sanitizer->add_twentytwenty_modals(Array) 
#1 .../includes/templates/class-amp-content-sanitizer.php(117): AMP_Core_Theme_Sanitizer->sanitize() 
#2 .../includes/class-amp-theme-support.php(2275): AMP_Content_Sanitizer::sanitize_document(Object(DOMDocument), Array, Array) 
#3 .../includes/class-amp-theme-support.php(1963): AMP_Theme_Support::prepare_response(‘<!DOCTYPE html>…’) 
#4 [internal function]: AMP_Theme_Support::finish_output_buffering(‘<!DOCTYPE html>…’, 9) 
#5 /home2/gustavors/public_html/wp-includes/functions.php(4483): ob_end_flush() 
#6 /home in .../includes/sanitizers/class-amp-core-theme-sanitizer.php on line 1597

While not stated in the DOMNodeList docs, it appears that the count() method was introduced in PHP 7.2 because this is when the class became Countable:

The Countable interface is implemented and returns the value of the length property.

Therefore, to be compatible with older versions of PHP we must use length instead of count().

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@westonruter westonruter added this to the v1.4.1 milestone Nov 13, 2019
@googlebot googlebot added the cla: yes Signed the Google CLA label Nov 13, 2019
@pierlon pierlon self-requested a review November 13, 2019 19:54
Copy link
Copy Markdown
Contributor

@pierlon pierlon left a comment

Choose a reason for hiding this comment

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

These are the only instances of where count() is used to return the value of length, so the reported issue shouldn't occur in the future.

@westonruter westonruter merged commit 980375c into develop Nov 13, 2019
@westonruter westonruter deleted the fix/domnodelist-count-usage branch November 13, 2019 20:23
westonruter added a commit that referenced this pull request Nov 14, 2019
* tag '1.4.1': (26 commits)
  Bump 1.4.1
  Update screenshots for 1.4.1
  Fix expected image name after upstream change (#3749)
  Use length property instead of count() method on DOMNodeList (#3727)
  Improve display of validation errors for scripts (#3722)
  Conditionally run E2E tests (#3723)
  Tidy up validation error details (#3721)
  Bump 1.4.1-RC1
  Default to the homepage instead of fetching the first AMP compatible post to customize (#3715)
  Add missing space after sentence (#3720)
  Include text content of style element in validation error (#3717)
  Use bitwise operator.
  Check if element is not in top toolbar.
  Fix user select for meta date and author
  Allow right click for meta blocks
  Fix summarizing error sources both parent theme and child theme (#3709)
  Fix identifying sources for validation errors coming child themes (#3708)
  Fix failing E2E tests (#3707)
  Remove amp_validate query var from Validated URL 'View' row action (#3706)
  Escape instances of unescapeed output in AMP settings screen code (#3703)
  ...
westonruter added a commit that referenced this pull request Nov 15, 2019
…ve-duplicate-amp-scripts

* 'develop' of github.com:ampproject/amp-wp:
  Manually add copies of AMP scripts rather than using wp_print_scripts()
  Add duplicate scripts to head as well
  Add test for removing duplicate scripts
  Bump stable tag to 1.4.1
  Update screenshots for 1.4.1
  Fix expected image name after upstream change (#3749)
  Use length property instead of count() method on DOMNodeList (#3727)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Signed the Google CLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants