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

Test if set_time_limit exists #5675

Merged
merged 3 commits into from Sep 27, 2023
Merged

Test if set_time_limit exists #5675

merged 3 commits into from Sep 27, 2023

Conversation

V-E-O
Copy link
Contributor

@V-E-O V-E-O commented Sep 27, 2023

…abled functions

Closes #4982

Changes proposed in this pull request:
-@set_time_limit to function_exists then set_time_limit

How to test the feature manually:
1.purge and optimize db
2.refresh and actualize feeds

Pull request checklist:

  • clear commit messages
  • code manually tested
  • unit tests written (optional if too hard)
  • documentation updated (documentation not affected)

Additional information can be found in the documentation.

@Alkarex Alkarex added this to the 1.22.0 milestone Sep 27, 2023
@Alkarex Alkarex changed the title Fix @set_time_limit as @-operator after PHP8 no longer suppresses dis… Test if set_time_limit exists Sep 27, 2023
@@ -169,7 +169,9 @@ public function optimizeAction(): void {
Minz_Request::forward($url_redirect, true);
}

@set_time_limit(300);
if (function_exists('set_time_limit')) {
@set_time_limit(300);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@set_time_limit(300);
$isTimeLimited = set_time_limit(300);
if ($isTimeLimited === false) {
continue;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

You can add the logic in function

Copy link
Contributor Author

@V-E-O V-E-O Sep 27, 2023

Choose a reason for hiding this comment

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

@ColonelMoutarde After PHP8, with function set_time_limit disabled, set_time_limit (or even decorated with @) will cause fatal termination of script.

Prior to PHP 8.0.0, it was possible for the @ operator to disable critical errors that will terminate script execution. For example, prepending @ to a call of a function which did not exist, by being unavailable or mistyped, would cause the script to terminate with no indication as to why.

from https://www.php.net/manual/en/language.operators.errorcontrol.php

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, https://www.php.net/manual/en/function.set-time-limit.php set_time_limit is available for all php versions from 4 to 8.
The @ is not necessary, especially as you're checking that the function is present just beforehand. ;)

Copy link
Member

@Alkarex Alkarex Sep 28, 2023

Choose a reason for hiding this comment

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

set_time_limit is available for all php versions from 4 to 8.

Partially wrong: any function can be disabled, and we have several examples of providers doing so for that specific function

The @ is not necessary

Wrong: there might be a warning with PHP < 8 (the function would exist, but would be disabled). See https://php.watch/versions/8.0/disable_functions-redeclare

@Alkarex Alkarex merged commit b82c41f into FreshRSS:edge Sep 27, 2023
1 check passed
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.

[BUG] PHP 8 disabled function behavior has changed
4 participants