Fixed PHPStan error in Mage_HTTP_Client_Socket and Mage_HTTP_Client_Curl

[fballiano]

I rewrote the list($key, $val) = explode("=", $values[0]);

But it seems to me that the whole Mage_HTTP* is never used, should we at least deprecate it? or remove it in next?

[justinbeaty]

I guess PHPStan is a bit wrong here, because $val very well could have been null:

list($key, $val) = explode('=', 'foo');

var_dump($key); // string(3) "foo"'
var_dump($val); // NULL

I'd be curious if it throws the error if you used if ($val === null). Alternatively, you could use if (!array_key_exists(1, $keyValueArray)) (since index 0 will always be set). Or even just if (count($keyValueArray) >= 2).

---

It seems like some people might be using this library, see: #913. However, I support the removal of these classes (move it to an external repo, I suppose.) We should encourage people to use the power of Composer here, and just install something like Guzzle or one of the many other helper libraries. These built-in libraries like this are so old-school.

[fballiano]

@justinbeaty I've removed the first unnecessary check. I wouldn't care too much about phpstan sincerely because it also is not perfect... anyway since nowadays if you don't do phpstand you're not cool... it bothers me to have a million errors.

what if we deprecate all these classes in lib/Mage, copy them to a separate repo and remove them from next?

[justinbeaty]

what if we deprecate all these classes in lib/Mage, copy them to a separate repo and remove them from next?

I think it's okay to deprecate / remove. It's an easy fix for someone to add back if they used it.

There's probably a lot more in that lib/Mage folder we can remove...

[fballiano]

I've removed the whole lib/Mage and see no error browsing around...

[justinbeaty]

I think Mage_Exception might be the only used class from there.

These are the classes I see defined:

$ grep -Roh 'class Mage[_A-Za-z]*' lib/Mage | cut -d' ' -f 2
Mage_System_Ftp
Mage_System_Dirs
Mage_System_Args
Mage_HTTP_Client
Mage_HTTP_Client_Curl
Mage_HTTP_Client_Socket
Mage_Exception
Mage_Archive
Mage_Archive_Tar
Mage_Archive_Gz
Mage_Archive_Bz
Mage_Archive_Abstract
Mage_Archive_Helper_File
Mage_Archive_Helper_File_Gz
Mage_Archive_Helper_File_Bz
Mage_DB_Exception
Mage_DB_Mysqli
Mage_Xml_Parser
Mage_Xml_Generator
Mage_Cache_Backend_File
Mage_Cache_Backend_Redis

[fballiano]

you're considering lib/Mage too I think, cause I can see only these occurrences:

which should be converted to Mage_Core_Exception I think

[fballiano]

ps: same think could be for lib/Magento... more or less..

[justinbeaty]

you're considering lib/Mage too I think, cause I can see only these occurrences:

I meant that Mage_Exception was the only class defined in lib/Mage that was used outside of lib/Mage. We're saying the same thing.

To figure this out, I grepped for each of those classes in app/ and found nothing. I also grepped for mage/archive etc, but I don't think they're loadable via getModel anyway.

I think we could rewrite Mage_Exception -> Mage_Core_Exceptoin too. I don't know why there's two, probably just legacy crap. If needed, we can even do this somewhere:

if (!class_exists('Mage_Exception')) {
  class Mage_Exception extends Exception {}
}

[fballiano]

legacy crap 😂

[fballiano]

I'd do all of these things in separate PRs anyway

[justinbeaty]

I'd do all of these things in separate PRs anyway

Yeah, so then this PR is fine to me. approved.

[sreichel]

I guess PHPStan is a bit wrong here, because $val very well could have been null:

list($key, $val) = explode('=', 'foo');

var_dump($key); // string(3) "foo"'
var_dump($val); // NULL

I guess phpstan "thinks correct" here, but should give a hint about error in list().

The given example throws a notice on php7 (Notice: Undefined offset: 1 in list(....) = explode(...)) and a warning for php8 (Warning: Undefined array key 1)

Suggestion:

list($key, $val) = array_pad(explode('=', $values[0]), 2, null);

https://phpstan.org/r/6cef421c-5e0e-411e-9782-3a1f17b8aa6d

[fballiano]

i've refactored this one

[sreichel]

LGTM, but do we need !strlen($key)?

[fballiano]

mmm I thought it would be better to check the key too and not only the value, in case after the trim it becomes empty, dunno.

[sreichel]

Port to v19?