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

refactor: remove unused non-empty array in RequestTrait #7606

Closed

Conversation

paulbalandan
Copy link
Member

@paulbalandan paulbalandan commented Jun 22, 2023

Description
Brought by RemoveUnusedNonEmptyArrayBeforeForeachRector but manually made as I cannot use rector on Windows.

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
Copy link
Member

kenjis commented Jun 22, 2023

The $proxyIPs value may be an empty string if Config\App is not updated.

* Comma-separated: '10.0.1.200,192.168.5.0/24'
* Array: ['10.0.1.200', '192.168.5.0/24']
*
* @var string|string[]
*/
public $proxyIPs = '';

In this case, this change causes:

ErrorException
foreach() argument must be of type array|object, string given
SYSTEMPATH/HTTP/RequestTrait.php at line 82

@kenjis kenjis added the refactor Pull requests that refactor code label Jun 22, 2023
@paulbalandan
Copy link
Member Author

Ok. Judging by how $this->proxyIPs is injected with value, L68 should be revised to check that $proxyIPs should be an array even if empty or not.

@kenjis
Copy link
Member

kenjis commented Jun 24, 2023

It is better to set [] if $proxyIPs = ''. Many devs do not use Proxy at all.

Or add "$proxyIPs must be an array" to Mandatory File Changes in the upgrade guide.

@kenjis
Copy link
Member

kenjis commented Jun 26, 2023

I sent another PR #7620
and will make sure $proxyIPs is an array in 4.4. See #7621

@kenjis kenjis closed this in #7620 Jun 26, 2023
@paulbalandan paulbalandan deleted the refactor-request-trait branch June 26, 2023 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Pull requests that refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants