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

PHP 8.1 compatibility #4554

Merged
merged 2 commits into from
Sep 10, 2021
Merged

PHP 8.1 compatibility #4554

merged 2 commits into from
Sep 10, 2021

Conversation

Deltik
Copy link
Member

@Deltik Deltik commented Sep 4, 2021

Motivation and Context

PHP 8.1 about to be released, but e107 is not compatible with various deprecations and behavior changes.

Description

This pull request fixes all PHP 8.1 errors caught by the existing tests. Specifically:

Deprecations

  • strftime() has been replaced with a polyfill based on DateTime.
  • Explicit type casts/assertions added where required by PHP 8.1
  • filter_var(…, FILTER_SANITIZE_STRING) replaced with strip_tags()
      or HTML entity encoding of quotation marks, depending on a guess of
      what the intended "sanitization" was
  • http_build_query() usage type mismatches fixed
  • Removed usages of the FILE_TEXT constant
  • To avoid breaking PHP 5.6 compatibility (function return types),
      e_session_db no longer implements SessionHandlerInterface.
      Instead, the alternative non-OOP invocation of
      session_set_save_handler() is used instead to apply the session
      handler.
  • The shim for strptime() still calls the native function if available
      but now suppresses the deprecation warning.

Behavior Changes

  • e_db_pdo explicitly asks for PDO::ATTR_STRINGIFY_FETCHES to
      maintain consistent behavior with past versions of PHP.
  • e_db_mysql explicitly sets mysqli_report(MYSQLI_REPORT_OFF) to
      maintain consistent behavior with past versions of PHP.

Other

  • Removed pointless random number generator seed from banner plugin
  • Workaround for COUNT(*) SQL query in
      validatorClass::dbValidateArray() without a proper API for avoiding
      SQL injection

How Has This Been Tested?

The changes make the existing tests pass on all PHP major versions since version 5.6.

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist

* `strftime()` has been replaced with a polyfill based on `DateTime`.
* Explicit type casts/assertions added where required by PHP 8.1
* `filter_var(…, FILTER_SANITIZE_STRING)` replaced with `strip_tags()`
  or HTML entity encoding of quotation marks, depending on a guess of
  what the intended "sanitization" was
* `http_build_query()` usage type mismatches fixed
* Removed usages of the `FILE_TEXT` constant
* To avoid breaking PHP 5.6 compatibility (function return types),
  `e_session_db` no longer implements `SessionHandlerInterface`.
  Instead, the alternative non-OOP invocation of
  `session_set_save_handler()` is used instead to apply the session
  handler.
* The shim for `strptime()` still calls the native function if available
  but now suppresses the deprecation warning.

* `e_db_pdo` explicitly asks for `PDO::ATTR_STRINGIFY_FETCHES` to
  maintain consistent behavior with past versions of PHP.
* `e_db_mysql` explicitly sets `mysqli_report(MYSQLI_REPORT_OFF)` to
  maintain consistent behavior with past versions of PHP.

* Removed pointless random number generator seed from `banner` plugin
* Workaround for `COUNT(*)` SQL query in
  `validatorClass::dbValidateArray()` without a proper API for avoiding
  SQL injection
@Deltik Deltik changed the title PHP 8.0 compatibility PHP 8.1 compatibility Sep 4, 2021
@Deltik Deltik requested review from CaMer0n and Moc September 4, 2021 13:11
@Deltik Deltik marked this pull request as draft September 4, 2021 13:11
@Deltik Deltik marked this pull request as ready for review September 6, 2021 15:23
@Deltik Deltik force-pushed the php-8.1 branch 3 times, most recently from 4dab7d2 to 2088292 Compare September 6, 2021 16:47
@codeclimate
Copy link

codeclimate bot commented Sep 6, 2021

Code Climate has analyzed commit 2088292 and detected 6 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1
Bug Risk 5

The test coverage on the diff in this pull request is 86.8% (80% is the threshold).

This pull request will bring the total coverage in the repository to 34.2% (0.6% change).

View more on Code Climate.

@@ -107,7 +107,7 @@
{
$pid = intval(varset($_POST['pid'], 0)); // ID of the specific comment being edited (nested comments - replies)
$row = array();
$authName = filter_var($_POST['author_name'],FILTER_SANITIZE_STRING);
$authName = e107::getParser()->filter($_POST['author_name'], 'str');
Copy link
Member

Choose a reason for hiding this comment

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

@Deltik Maybe I missed something, doesn't this do exactly the same as the code you replaced?

public function filter($text, $type = 'str', $validate = false)

...



if($validate === false)
				{
					$filterTypes = array(
						'int'   => FILTER_SANITIZE_NUMBER_INT,
						'str'   => FILTER_SANITIZE_STRING, // no html.
						'email' => FILTER_SANITIZE_EMAIL,
						'url'   => FILTER_SANITIZE_URL,
						'enc'   => FILTER_SANITIZE_ENCODED
					);
				}
				```

Copy link
Member Author

Choose a reason for hiding this comment

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

e_parse::filter() has been reworked not to use filter_var(…, FILTER_SANITIZE_STRING). (Source)

According to the documentation of the deprecation of FILTER_SANITIZE_STRING, the usage of the filter is unclear. I looked at e107's usages of that filter and found that some of them expected strip_tags() behavior while others thought that the filter would make the provided string safe to quote in an SQL query. As there is no way for a filter to concatenate arbitrary strings safely for SQL queries, I decided to make e_parse::filter(…, 'str', false) just mean strip_tags().

I replaced the confounded usages of filter_var(…, FILTER_SANITIZE_STRING) with guesses for what the usages' intentions were:

  • Preventing SQL injection – Example
  • Remove HTML tags – Example
  • Kinda hide quotation marks for use in SQL – Example
  • No apparent reason at all – Example

@CaMer0n
Copy link
Member

CaMer0n commented Sep 6, 2021

Thank you!! Looks good, please see comment about filter().

@Deltik Deltik requested a review from CaMer0n September 6, 2021 20:41
@CaMer0n CaMer0n merged commit 3e52f29 into e107inc:master Sep 10, 2021
@Deltik Deltik deleted the php-8.1 branch September 11, 2021 13:01
@CaMer0n
Copy link
Member

CaMer0n commented Sep 23, 2021

@Deltik This commit introduced a bug into the form handler.

git.exe bisect good
2088292 is the first bad commit
commit 2088292
Author: Nick Liu
Date: Sat Sep 4 15:06:19 2021 +0200

Specifically it renders the following invalid HTML in the batch options and elsewhere.

I previously committed a fix for the pop-up confirmation on the admin area cache delete button, which had a similar issue. ( 632f335)

It appears the parsing of the $options['other'] value has been corrupted (see quick-fix commit above)

<select name="etrigger_batch" id="etrigger-batch" class="tbox form-control input-large select batch e-autosubmit reset" data-original-title="" title="">
<option value="">With selected...</option>
<option value="copy" class="ui-batch-option class" style="&quot;padding-left:" 15px&quot;="">Copy</option>
<option value="delete" class="ui-batch-option class" style="&quot;padding-left:" 15px&quot;="">Delete</option>
<option value="export" class="ui-batch-option class" style="&quot;padding-left:" 15px&quot;="">Export</option>	

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants