gen_stub: various simplifications and clean up #16291
Conversation
|
gen_stub.php required PHP 7.4, see: Lines 152 to 153 in dd0ced3 readonly is only in PHP 8.1. |
Hmm, I hadn't realized that. But, do we really need to continue supporting running the stub generation in 7.4? Given that 8.1 is the oldest version of PHP still supported (albeit with only security fixes) I think it would make sense to update the minimum to 8.1 I can update this patch to change the requirement, or split the readonly changes to be done separate if that warrants further discussion. |
The following one-line methods, only used in `SimpleType::fromValue()`, were inlined: * `SimpleType::bool()` * `SimpleType::int()` * `SimpleType::float()` * `SimpleType::string()` * `SimpleType::array()` * `SimpleType::object()` Doing so removes an unneeded level of indirection and helps simplify the class.
Both `ArgInfo::setTypes()` and `ReturnInfo::setTypes()` were private methods only called in the applicable class's constructor. They had no special logic that benefited from existing as a separate method, and just added a level of indirection. Inline the uses and remove the methods.
…method Protected method not overridden in any subclasses, so could be made private, but the method is short enough and simple enough that it can just be inlined.
…Null()` `Type::tryToSimpleType()` tries to convert a type holding multiple simple types into a single simple type, with the following logic - if all of the inner types represent `null`, return the first of those - if all but one of the inner types represent `null`, return the non-null type - otherwise, return `null` Previously, it did this with a helper method `::getWithoutNull()`, that constructed a new `Type` containing only the inner types that did not represent `null`. However, the only thing the newly created object was used for was extracting the types it contains, so the actual object creation just adds overhead. Merge `Type::getWithoutNull()` into `Type::tryToSimpleType()` and clean up to avoid creating an unneeded object.
Instead of using integers that then need to be converted to the string representation via `::getSendByString()`, just always store a string, eliminating the conversion method and replacing it with property access.
* `FuncInfo::hasParamWithUnknownDefaultValue()` * `FuncInfo::getFramelessFunctionInfosName()` * `ClassInfo::getClassSynopsisReference()`
The objects are immutable, and thus can be safely reused when held by an object that gets cloned. Remove the unneeded cloning, and document this fact about the class, noting that the property should be considered `readonly`. Not actually using `readonly` to maintain PHP 7.4 support.
`Type::$types` is typed as an array, and thus can never be `null`. There is no need to have code to handle such a case.
In the hopes that it will be clearer that some of the custom clone handling can be removed the way it was for `ExposedDocComment`, instances of which were immutable, document a bunch of properties of other classes as read-only.
DanielEScherzer
force-pushed
the
stub-simplify
branch
from
66f6f83 to
b6d3607
Compare
For now I changed it so that this patch just identifies read-only properties with |
There was a problem hiding this comment.
Thanks for the cleanup! Most of them made sense to me, only a few changes seemed less useful for me. I don't really like how readonly modifier looks like now between the visibility modifier and the type, but I get why it was added there...
Please note that in the meanwhile the minimum PHP dependency was increased to PHP 8.0 if I saw it correctly.
| @@ -2605,11 +2603,6 @@ protected function getVariableTypeName(): string | |||
| return "constant"; | |||
| } | |||
|
|
|||
| protected function getVariableTypeCode(): string | |||
There was a problem hiding this comment.
VariableLike::getTypeCode() should have called getVariableTypeCode() instead of getVariableTypeName(), but since the difference is minimal, I'm fine the removal of getVariableTypeCode().
I swear I saw some commit related to this a few weeks ago, but as I've just seen it, the configure script still has the 7.4 requirement. Then sorry for the false alarm. |
Each commit can be reviewed independently, and should have no functional changes