Type-safety cleanup of src/Sql (mago analyze) with phpcs/phpstan compliance#3
Draft
simon-mundy wants to merge 22 commits into
Draft
Type-safety cleanup of src/Sql (mago analyze) with phpcs/phpstan compliance#3simon-mundy wants to merge 22 commits into
simon-mundy wants to merge 22 commits into
Conversation
- Add ExpressionDataAssertionsTrait with argumentAt()/predicateEntryAt() - Replace unchecked fixed-index access in Predicate tests and SelectTest - Document Expression::getParameters() return type
- Add ConcreteSql exposing AbstractSql protected methods with typed wrappers - Replace ReflectionMethod invocations with direct calls in AbstractSqlTest - Convert Ddl constraint/column tests to argumentAt() - Guard preg_match captures and cast key() results
- Coalesce keyed state and decorator lookups inside assertions - Assert decorator tuple keys in AbstractSqlFunctionalTestCase
- Port analyzer, linter, and formatter config from global settings - Pin php-version 8.2 - Register PHPUnit setUp/setUpBeforeClass as class initializers - Scope invalid-enum-case-value ignore to src/Sql/
- Define the specification/parameter/column array shapes as @phpstan-type on AbstractSql and import them where consumed - Tighten ExpressionInterface::getExpressionData() values, Expression and the Argument flatten helper to list<ArgumentInterface> - Widen processExpressionParameterName() to accept null bound values and return a non-null name - Rework createSqlFromSpecificationAndParameters() to interpret the spec DSL through typed locals instead of unchecked array access - Type the Join specification structure (JoinName/JoinSpecification) and guard the iterator and constructor edges - Type Select properties, process* returns and setter params; guard the mixed access in the process bodies and fix the resolveTable tuple type Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
- Move aliased-array table stripping into AbstractSql::resolveTable so Insert/Update/Delete no longer reimplement it; Select stays the only override, as it alone preserves the alias as a [table, fromTable] tuple - Widen resolveColumnValue() to accept float bound values - Type the Insert/Update/Delete specifications, table, column and predicate members and narrow the spec format to string at each str_replace call - Guard driver-dependent parameter binding, cast column keys used as identifiers, and document the genuinely-mixed value maps Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
Replaces the repeated `@var string` narrowing of the specifications map in Insert/Update/Delete with a single typed accessor, so the format string is narrowed once at a named boundary rather than at each call. Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
- Type Combine's combine-spec list, combine input, and narrow the mixed getRawState() column results in alignColumns() - Type Sql's table member/accessors, drop the dead null branches from getAdapter()/getSqlPlatform(), and render the table safely in the single-table guard message - Widen the Insert/Update/Delete constructors to accept the aliased-array table their into()/table()/from() already accept Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
- Type the Expression parameter shape (ExpressionParameter) and count placeholders with substr_count() instead of the str_replace out-param - Widen Argument\Values to accept any-keyed scalar arrays (it reindexes) - Make AbstractPlatform's subject nullable, type the decorator map, and guard the resolved decorator before prepareStatement()/getSqlString() Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
- Narrow the fluent Predicate identifier params to the real contract (string|ArgumentInterface) the concrete predicates accept; value operands keep bool|float|int|string so value escaping is unchanged - Type PredicateSet's predicate-entry list, the addPredicates spec input, and In's value set - Drop PredicateSet::getExpressionData's redundant single-predicate branch (the general loop yields the same result) and fix a latent bug where a non-predicate Expression was added instead of a Predicate\Expression Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
- Type the column option maps, constraint/index column lists (list<string>)
and the create/alter member collections
- Align Column subclass constructors' default to the setDefault() contract;
type Check's expression as string (the ExpressionInterface form was
declared but unimplemented)
- Make the Ddl process methods take a non-null platform, drop the psalm
list{0?:...} return tags mago cannot parse, and read plain-string specs
through specificationString()
- Replace index-based column loops with foreach and guard array truthiness
Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
- Narrow the where()/having() array docblocks to the PredicateSpecValue contract they hand to PredicateSet::addPredicates, importing the shape rather than passing the wider array<array-key, mixed> - Type Argument::values() and ArgumentInterface::getValue() array shapes to the scalar-or-null values they actually carry Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
A Select subquery is a valid where() value, so widen PredicateSpecValue (and Predicate::expression()/Expression parameters) to include SqlInterface. Route SqlInterface through SelectArgument in Expression::setParameters, which the wrapper already declared it accepts, closing a latent TypeError where a subquery placeholder parameter fell through to Value. Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
- Add readArrayAttribute() so reflection reads narrow to array at one boundary instead of a mixed cast at every call site - Give AbstractSqlFunctionalTestCase phpstan-type shapes for its data providers, dropping the per-iteration mixed assignments and suppressions - Convert DeleteTest predicate assertions to the predicateEntryAt extractor - Narrow Delete::getRawState() off mixed to its real union Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
- Read raw-state objects through /** @var Type */ or the ?Type magic property, narrowing lazy where/having/joins before use - Narrow list-index and raw-state key access; read the specifications attribute through readArrayAttribute - @mago-expect the deliberate invalid-argument/magic-property exception cases; scope unused shared data-provider parameters - Widen Select::quantifier()'s @param to ExpressionInterface to match its native signature (the docblock was narrower than the accepted type) Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
Complete the mago cleanup of the remaining Sql test files: read raw state and reflected attributes through typed @var/?? null guards, narrow list and string-key access, type closure and helper signatures, and replace psalm/inspection suppressions with @mago-expect on the deliberately invalid expectException calls and unmodellable magic-property reads. Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
- Read raw state and reflected predicates through readArrayAttribute and /** @var Type */, narrowing lazy where and nullable getSql before use - Type the falsy-parameter data provider off mixed - Replace psalm/inspection suppressions with @mago-expect on the deliberately invalid and deprecated-variadic constructor calls Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
- Define PredicateSpecValue as PredicateInterface|ExpressionParameter, importing the shared alias instead of restating (and overrunning the line limit with) the full union - Use the array<value> short form for the Expression parameter alias - Regroup the docblock annotations and restore end-of-file newlines the analyzer widening had disturbed Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
These are project-wide API-surface policies (179 sites across src and test) the fork has not adopted; disable the two checks so the analyzer reports only actionable issues rather than the whole legacy surface. Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
In::setValueSet's [value => ArgumentType] form was never interpreted: Argument\Values is scalar-only, stores the entries opaquely and would TypeError if rendered to SQL. Remove the speculative block (the scalar value-set case already covers the real behaviour) and rename the test. Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
- Match InsertIgnore::$specifications to Insert's type (property types are invariant), drop the now-redundant getSqlPlatform() null assertion, and remove a stale inline @PHPStan-Ignore that the improved typing made moot - Regenerate the baseline: clear entries the cleanup fixed, and record the remaining mago/phpstan disagreements (defensive ??/?- forms mago requires), the deliberate __get/TypeError tests, and the deferred Platform issues Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
phpstan resolves $select->where inconsistently across versions (local via __get's non-null union, CI via the possibly-null @Property), so the nullsafe form produced a version-sensitive baseline entry that failed on CI's latest phpstan. Route the predicates through where(Closure), whose argument is an unambiguously non-null Where, and drop the fragile nullsafe baseline entries. Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Using Mago to increase type-safety and help keep code as documented and clean as possible. Runtime behaviour is unchanged apart from two fixes below.
Changes
@phpstan-type/@phpstan-import-typealiases on class definitions (AbstractSql,Join,PredicateSet,Expression, etc.)getRawState()returns come offmixedwhere the union is small enough to be usefulExpression::setParametersnow routesSqlInterfacevalues throughSelectArgument, which already said it accepted them. Previously a subquery as a predicate/expression parameter wouldTypeErrorinValueTests
predicateEntryAt/argumentAtfor cleaner handlingInTest's "typed value-set" assertions.Argument\Valuesis scalar-only and never read thoseArgumentTypehintsExcluded
src/Sql/Platform/Platform.php, ~19 analyzer issues which may need redesignunhandled-thrown-typesetting would mean adding@throws RuntimeException. Maybe defer for now?