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

fix: is_cli() returns true when $_SERVER['HTTP_USER_AGENT'] is missing #5393

Merged
merged 2 commits into from
Dec 10, 2021

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Nov 26, 2021

Description
Fixes #5336

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • [] User guide updated
  • Conforms to style guide

@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Nov 26, 2021
@MGatner
Copy link
Member

MGatner commented Nov 26, 2021

These were changes pretty recently by @paulbalandan to address other issues - he should take a look.

@kenjis kenjis requested a review from paulbalandan November 26, 2021 23:02
Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

I'm not sure how to approach this. The recent changes in the is_cli() method were to account for cron jobs running in php-cgi (#4699 fixed by #4725), edge case in #4739, as well as #4843 and #4848. Obviously, accepting this fix would fail cronjobs in cgi fixed previously.

I still believe the proper fix for this is in my #5336 (comment) by expanding the isset calls. Also, seen in StackOverflow https://stackoverflow.com/a/25967493

@kenjis
Copy link
Member Author

kenjis commented Dec 3, 2021

Thanks.

#4699 is cgi-fcgi.
#4739 does not show why. What's the issue to solve?
#4843 is fpm-fcgi.
#4848 is fpm-fcgi.

Obviously, accepting this fix would fail cronjobs in cgi fixed previously.

I agree with it.
It seems there are users who do not use php-cli with cron.

@kenjis
Copy link
Member Author

kenjis commented Dec 3, 2021

@paulbalandan I added conditions.

I removed HTTP_USER_AGENT and argv conditions, because #5336 has argv and no HTTP_USER_AGENT with Apache environment.

@paulbalandan
Copy link
Member

#4739 was raised by @najdanovicivan in the Slack channel. I cannot remember the issue then, but adding the argv checks fixes his issue.

@kenjis
Copy link
Member Author

kenjis commented Dec 7, 2021

Searched slack, and I found Ivan said the following causes the issue.

if (! isset($_SERVER['REMOTE_ADDR'], $_SERVER['HTTP_USER_AGENT']))
{
   return true;
}

And said $_SERVER[‘argv’] is not set.

The following is the fixed code at that time:

		if (PHP_SAPI === 'cli')
		{
			return true;
		}

		if (defined('STDIN'))
		{
			return true;
		}

		if (stristr(PHP_SAPI, 'cgi') && getenv('TERM'))
		{
			return true;
		}

		if (! isset($_SERVER['REMOTE_ADDR'], $_SERVER['HTTP_USER_AGENT']) && isset($_SERVER['argv']) && count($_SERVER['argv']) > 0)
		{
			return true;
		}

		// if source of request is from CLI, the `$_SERVER` array will not populate this key
		return ! isset($_SERVER['REQUEST_METHOD']);

@kenjis
Copy link
Member Author

kenjis commented Dec 7, 2021

@najdanovicivan Do you have any problem with this PR?

@najdanovicivan
Copy link
Contributor

@najdanovicivan Do you have any problem with this PR?

All good here. I had issues with some web requests being treated as CLI due to missing HTTP_USER_AGENT

Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

Since all involved parties are okay with this, then it is good to go.

@kenjis kenjis merged commit b5478b3 into codeigniter4:develop Dec 10, 2021
@kenjis kenjis deleted the fix-is-cli branch December 10, 2021 00:21
@MGatner
Copy link
Member

MGatner commented Dec 10, 2021

💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: is_cli() returns true on Apache
4 participants