Skip to content

Comments

[4.4] set_time_limit php 8#41523

Merged
laoneo merged 5 commits intojoomla:4.4-devfrom
brianteeman:set_time_limit
Sep 8, 2023
Merged

[4.4] set_time_limit php 8#41523
laoneo merged 5 commits intojoomla:4.4-devfrom
brianteeman:set_time_limit

Conversation

@brianteeman
Copy link
Contributor

Pull Request for Issue #38876 .

Summary of Changes

some hosts disable set_time_limit in php

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.

This PR replaces the @ with a function_exists check

Testing Instructions

code review or in php.ini
disable_functions = set_time_limit

Actual result BEFORE applying this Pull Request

Admin dashboard loads with an error
finder index ends with an error

Expected result AFTER applying this Pull Request

everything works

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@richard67
Copy link
Member

@brianteeman As this PR is for 4.4-dev: Have you checked if it works with PHP 7.4? I'm asking because of this info I get from here https://php.watch/versions/8.0/disable_functions-redeclare , especially the last sentence :

function_exists() returns false for disabled functions
function_exists() function now returns false for functions disabled with disable_functions INI directive. This is because internally, disabled functions do not even make to the internal functions table.
Prior to PHP 8, to check if a function is disabled, one would need to query the INI setting, or use the return value of get_defined_functions() function.

Could you check?

@brianteeman
Copy link
Contributor Author

@richard67 I assume it works otherwise we have a problem with the existing use

// Let's try to avoid time-outs
if (\function_exists('set_time_limit')) {
set_time_limit(0);

@brianteeman
Copy link
Contributor Author

I just retested this PR with php 7.4.9 and set_time_limit disabled and no problems

@richard67
Copy link
Member

I just retested this PR with php 7.4.9 and set_time_limit disabled and no problems

Ok, thanks for checking.

@Quy
Copy link
Contributor

Quy commented Sep 1, 2023

I have tested this item ✅ successfully on 2daf4cf


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/41523.

@richard67
Copy link
Member

Is there a reason why this PR is made for 4.4-dev and not 4.3-dev? To me it seems to be a bug fix.

@brianteeman
Copy link
Contributor Author

No reason

@brianteeman brianteeman changed the base branch from 4.4-dev to 4.3-dev September 2, 2023 20:00
@brianteeman brianteeman changed the base branch from 4.3-dev to 4.4-dev September 2, 2023 20:00
@laoneo
Copy link
Member

laoneo commented Sep 7, 2023

Do we have to copy the changes to the framework package as well @Hackwar?

@wilsonge
Copy link
Contributor

wilsonge commented Sep 7, 2023

Would be good to get this in the framework file system too if it’s effected but it’s not a hard requirement

@laoneo
Copy link
Member

laoneo commented Sep 8, 2023

@brianteeman are able to add the checks also to the framework package?

@laoneo laoneo self-assigned this Sep 8, 2023
@brianteeman
Copy link
Contributor Author

i will look at the framework now and do a pr there if needed as well

brianteeman added a commit to brianteeman/filesystem that referenced this pull request Sep 8, 2023
@brianteeman
Copy link
Contributor Author

brianteeman commented Sep 8, 2023

@wilsonge @laoneo I already did the framework changes last week at the same time I did this joomla-framework/filesystem#59

but it was rubbish so i did itagain joomla-framework/filesystem#60

@laoneo laoneo merged commit 61ff226 into joomla:4.4-dev Sep 8, 2023
@laoneo
Copy link
Member

laoneo commented Sep 8, 2023

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants