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.

Sign up for GitHub

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

Jump to bottom

PHPLIB-1250 Split encoders and fix psalm issues #46

Merged
merged 17 commits into from
Jan 29, 2024
Merged

PHPLIB-1250 Split encoders and fix psalm issues #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
8dcc886
Split encoders into multiple files
alcaeus Jan 22, 2024
02bfae8
Remove unnecessary factory method
alcaeus Jan 23, 2024
54d0aab
Add missing template annotation
alcaeus Jan 23, 2024
9444978
Add MixedArgument and MixedAssignment to psalm baseline
alcaeus Jan 23, 2024
8c11b8c
Fix type issues in query classes
alcaeus Jan 23, 2024
9192f90
Extract common builder code and fix template annotations
alcaeus Jan 23, 2024
6297c92
Update psalm baseline
alcaeus Jan 23, 2024
81cb397
Define ENCODE constant in OperatorInterface
alcaeus Jan 25, 2024
a913b72
Simplify canEncode check in BuilderEncoder
alcaeus Jan 25, 2024
5ce111a
Use strict comparison
alcaeus Jan 25, 2024
254cc4a
Fix checkstyle errors
alcaeus Jan 25, 2024
5795818
Fix wrong object type check
alcaeus Jan 25, 2024
ba2ac3c
Update todo for properties in interfaces
alcaeus Jan 25, 2024
a9b02b2
Fix return type of PipelineEncoder::encode
alcaeus Jan 25, 2024
24ae4d3
Update comment in OperatorEncoder
alcaeus Jan 25, 2024
ab93a1e
Optimise cached encoders structure
alcaeus Jan 26, 2024
c49a256
Add undefined case for Encode enum
alcaeus Jan 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion phpcs.xml.dist
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@
<exclude name="SlevomatCodingStandard.ControlStructures.UselessIfConditionWithReturn" />
<exclude name="SlevomatCodingStandard.Functions.StaticClosure" />
<exclude name="SlevomatCodingStandard.Functions.UnusedInheritedVariablePassedToClosure" />
<exclude name="SlevomatCodingStandard.Operators.DisallowEqualOperators" />

<!-- ********************* -->
<!-- Exclude broken sniffs -->
Expand Down
133 changes: 132 additions & 1 deletion psalm-baseline.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,134 @@
<?xml version="1.0" encoding="UTF-8"?>
<files psalm-version="5.15.0@5c774aca4746caf3d239d9c8cadb9f882ca29352">
<files psalm-version="5.20.0@3f284e96c9d9be6fe6b15c79416e1d1903dcfef4">
<file src="src/Builder/Encoder/AbstractExpressionEncoder.php">
<MixedAssignment>
Copy link
Member Author

Choose a reason for hiding this comment

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

A lot of MixedAssignment calls are the result of incomplete type annotations throughout the generated files.

<code>$val</code>
<code>$val</code>
<code>$value[$key]</code>
</MixedAssignment>
</file>
<file src="src/Builder/Encoder/CombinedFieldQueryEncoder.php">
<MixedAssignment>
<code>$filter</code>
<code>$filterValue</code>
</MixedAssignment>
</file>
<file src="src/Builder/Encoder/FieldPathEncoder.php">
<NoInterfaceProperties>
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixing this involves creating a new generator for field path classes so we can add a getName interface methods. It irks me that public properties cannot be part of the interface contract, but here we are.

<code><![CDATA[$value->name]]></code>
</NoInterfaceProperties>
</file>
<file src="src/Builder/Encoder/OperatorEncoder.php">
<MixedAssignment>
<code>$result</code>
<code>$result[]</code>
<code>$val</code>
<code>$val</code>
<code>$val</code>
<code>$val</code>
<code>$val</code>
</MixedAssignment>
</file>
<file src="src/Builder/Encoder/OutputWindowEncoder.php">
<MixedArgument>
<code>$result</code>
</MixedArgument>
</file>
<file src="src/Builder/Encoder/QueryEncoder.php">
<MixedArgument>
<code><![CDATA[$this->recursiveEncode($value)]]></code>
</MixedArgument>
<MixedAssignment>
<code>$subValue</code>
<code>$value</code>
</MixedAssignment>
</file>
<file src="src/Builder/Projection/ElemMatchOperator.php">
<MixedArgumentTypeCoercion>
Copy link
Member Author

Choose a reason for hiding this comment

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

MixedArgumentTypeCoercion errors are due to the query expression not using the correct array element types but instead using the array type. I tried to fix this, but the code generator doesn't accept a type like array<int|string>. This will have to be looked at separately.

<code>$query</code>
</MixedArgumentTypeCoercion>
</file>
<file src="src/Builder/Query.php">
<ArgumentTypeCoercion>
<code>$query</code>
</ArgumentTypeCoercion>
</file>
<file src="src/Builder/Query/ElemMatchOperator.php">
<MixedArgumentTypeCoercion>
<code>$query</code>
</MixedArgumentTypeCoercion>
</file>
<file src="src/Builder/Stage/AddFieldsStage.php">
<PropertyTypeCoercion>
<code>$expression</code>
</PropertyTypeCoercion>
<TooManyTemplateParams>
Copy link
Member Author

Choose a reason for hiding this comment

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

We use stdClass<foo|bar> to indicate an object with an unknown number of properties of a specific type. However, stdClass and object cannot be templated this way, which is why we're seeing this error. The stdClass{foo: int} syntax doesn't work for us here as we don't know the keys of the object, but only the expected value types. I've asked in the Symfony Slack to get more info about this and will update once I know more.

<code>stdClass</code>
</TooManyTemplateParams>
</file>
<file src="src/Builder/Stage/FacetStage.php">
<PropertyTypeCoercion>
<code>$facet</code>
</PropertyTypeCoercion>
<TooManyTemplateParams>
<code>stdClass</code>
</TooManyTemplateParams>
</file>
<file src="src/Builder/Stage/GeoNearStage.php">
<MixedArgumentTypeCoercion>
<code>$query</code>
</MixedArgumentTypeCoercion>
</file>
<file src="src/Builder/Stage/GraphLookupStage.php">
<MixedArgumentTypeCoercion>
<code>$restrictSearchWithMatch</code>
</MixedArgumentTypeCoercion>
</file>
<file src="src/Builder/Stage/GroupStage.php">
<PropertyTypeCoercion>
<code>$field</code>
</PropertyTypeCoercion>
<TooManyTemplateParams>
<code>stdClass</code>
</TooManyTemplateParams>
</file>
<file src="src/Builder/Stage/MatchStage.php">
<MixedArgumentTypeCoercion>
<code>$query</code>
</MixedArgumentTypeCoercion>
</file>
<file src="src/Builder/Stage/ProjectStage.php">
<PropertyTypeCoercion>
<code>$specification</code>
</PropertyTypeCoercion>
<TooManyTemplateParams>
<code>stdClass</code>
</TooManyTemplateParams>
</file>
<file src="src/Builder/Stage/SetStage.php">
<PropertyTypeCoercion>
<code>$field</code>
</PropertyTypeCoercion>
<TooManyTemplateParams>
<code>stdClass</code>
</TooManyTemplateParams>
</file>
<file src="src/Builder/Type/OutputWindow.php">
<DocblockTypeContradiction>
Copy link
Member Author

Choose a reason for hiding this comment

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

This and the RedundantConditionGivenDocblockType checks are psalm telling us that the condition is impossible due to the docblock. However, since we're being extra cautious here and any user not using psalm is able to send any types they want, I think it's fair to keep this around unless we want to accept potential fatal errors when users abuse this.

<code><![CDATA[! is_string($documents[1]) && ! is_int($documents[1])]]></code>
<code><![CDATA[! is_string($range[1]) && ! is_numeric($range[1])]]></code>
</DocblockTypeContradiction>
</file>
<file src="src/Builder/Type/QueryObject.php">
<MixedAssignment>
<code>$queries[$fieldPath]</code>
<code>$query</code>
</MixedAssignment>
<RedundantConditionGivenDocblockType>
<code><![CDATA[count($queriesOrArrayOfQueries) === 1 &&
isset($queriesOrArrayOfQueries[0]) &&
is_array($queriesOrArrayOfQueries[0]) &&
count($queriesOrArrayOfQueries[0]) > 0]]></code>
</RedundantConditionGivenDocblockType>
</file>
</files>
Loading
Loading