Remove unreachable backend code and fix two defects it was hiding - #281
Open
roncodes wants to merge 2 commits into
Open
Remove unreachable backend code and fix two defects it was hiding#281roncodes wants to merge 2 commits into
roncodes wants to merge 2 commits into
Conversation
Two defects surfaced while auditing unreachable code. GeocoderController::reverse validated coordinates only after converting them with Utils::getPointFromCoordinates(), which is typed `: Point` and falls back to Point(0, 0) for unusable input. The 'Invalid coordinates provided.' branch was therefore unreachable and garbage input silently reverse-geocoded Null Island. Resolve strictly instead so the guard works. An existing test asserted the old behaviour (reverseCalls[0] === [0.0, 0.0]) and now asserts the error fires with no lookup attempted. Casts/Polygon returned the raw geometry from its GeometryInterface arm while Casts/Point and Casts/MultiPolygon return a SpatialExpression. Only inserts survive the raw form, because SpatialTrait::performInsert wraps the attribute itself; there is no performUpdate, so on updates the value is bound directly and BaseBuilder::cleanBindings only expands a SpatialExpression into the WKT and SRID bindings that ST_GeomFromText(?, ?) needs. A bare Geometry has no __toString and cannot be bound. Align Polygon with the other two casts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Deletes branches no input can reach: concrete spatial-type arms shadowed by instanceof GeometryInterface; a duplicate isCoordinatesStrict test inside a branch already gated on it; Lalamove __callStatic's 'instance' case, which a declared public static never routes there; guards a type declaration makes redundant (getLocationAsPoint(): SpatialPoint, Request::date()'s Carbon, Find::httpResourceForModel() always resolving, Str::isUuid on a model); DriverController's company re-check after an early return; and the order-type fallback after an unconditional assignment, replaced by ?? at the assignment. Relocates Place::insertFromMixed's address-key check out of the is_string branch (empty() on a non-numeric string offset is always true) into the array branch, and moves the GoogleAddress arm above is_array||is_object so it is not swallowed and flattened. Makes coordsToCircle's loop exclusive so the ring is closed explicitly rather than by recomputing the 0-degree vertex, removing a duplicated point; output is otherwise identical. Keeps and annotates three guards that are deliberate: Casts/Point's SpatialExpression arm (the guard shadowing it skips the geometries bookkeeping, so the asymmetry is documented rather than erased), the globe-data ISO check (all 255 bundled features carry both codes), the post-validate photo check, and updateActivity's not-found guard pending the upstream core-api findByIdOrFail fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.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
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
Removes backend code that no input can reach, and fixes two latent defects found while proving that unreachability.
This came out of the coverage campaign in #277. Pushing
server/srcline coverage to 99.4%+ meant examining every remaining uncovered line, and a large share turned out not to be untested but unreachable — branches shadowed by a broader arm above them, guards on values a type declaration forbids, and fallbacks sitting after an unconditional assignment. No test can execute those, so they block the--fail-under=100gate permanently.Two of them were not harmless leftovers. They are fixed here.
The two commits are split deliberately so the behaviour changes can be reviewed apart from the deletions:
20fef6bb2aa143f3Defects fixed
1. Invalid coordinates silently reverse-geocoded Null Island
Internal\v1\GeocoderController::reverse()validated coordinates after converting them:getPointFromCoordinates()is typed: Pointand returnsnew Point(0, 0)for unusable input, so the guard never fired and garbage input was reverse-geocoded at0, 0instead of being rejected. Now resolved with the existing strict variant (getPointFromCoordinatesStrict(): ?Point) so the guard works as written.An existing test asserted the old behaviour —
reverseCalls[0] === [0.0, 0.0], i.e. it documented the Null Island lookup. It now asserts the error is returned and that no lookup is attempted.2. Polygon writes could not bind on update
Casts\Polygonreturned the raw geometry from itsGeometryInterfacearm, whileCasts\PointandCasts\MultiPolygonreturn aSpatialExpression. That is not cosmetic:SpatialTraitoverrides onlyperformInsert— there is noperformUpdate. Inserts survive either shape becauseperformInsert()wraps the attribute itself.BaseBuilder::cleanBindings()only expands aSpatialExpressioninto the WKT + SRID bindings thatST_GeomFromText(?, ?)requires. A bareGeometrypasses through untouched, and it has no__toString, so PDO cannot bind it.Casts\Polygonis now aligned with the other two.This is the riskiest hunk in the PR and the one worth the closest look. It is isolated in
20fef6bband can be reverted alone. Note that an insert-based test cannot validate it — the addedRulesAndCastsTestcase asserts at the binding level and was verified to fail on the old shape and pass on the new one.Unreachable code removed
Casts/{Polygon,MultiPolygon}instanceof GeometryInterface(Polygon → MultiLineString → GeometryCollection → Geometry implements GeometryInterface)Models/Place::createFromMixedisCoordinatesStrict()inside a branch already gated on it one arm aboveModels/Place::insertFromMixedinstanceof GoogleAddressarm sat belowis_array || is_object, which swallows every object — reordered so a GoogleAddress routes toinsertFromGoogleAddress()instead of being flattened to an arrayModels/Place::insertFromMixedis_string($place)branch, whereempty()on a non-numeric string offset is always true — moved into the array branch where anaddresskey can existIntegrations/Lalamove::__callStaticinstanceis a declaredpublic static, so PHP never routes it hereModels/ServiceRate::getLngLatFromPlacegetLocationAsPoint(): SpatialPoint, non-nullable, and aSpatialPointalways exposesgetLat/getLngInternal/v1/MetricsController::resolvePeriod$request->date(), which returns?Carbon; both??fallbacks yieldDateTimeResources/v1/{Maintenance,MaintenanceSchedule,WorkOrder,Order}JsonResourcefallbacks afterFind::httpResourceForModel(), which always resolves a class (falls back toFleetbaseResource)Resources/v1/{PurchaseRate,TrackingStatus}method_exists($this, 'loadMissing'); no class in the hierarchy declares it — it only resolves via__call, whichmethod_existscannot seeApi/v1/DriverController::createreturnalready guaranteed itApi/v1/OrderController::createtypefallback after line 72 assigns it unconditionally — replaced with?? 'transport'at the assignmentModels/Payload::setPlacecreateFromMixed(): ?Placealways yields a Model, so theStr::isUuid()arm and trailingelsewere both shadowedSupport/Utils::coordsToCircle0..360inclusive, so the ring was already closed and the closing push never ran — loop is now exclusive, which also removes a duplicated vertex. Output is otherwise byte-identical (verified: 121 points, closed, zero duplicate interior vertices)Kept and annotated, not deleted
Four guards are unreachable today but deliberately retained with
@codeCoverageIgnoreand an explanatory comment:Casts/Point'sSpatialExpressionarm. The guard that shadows it (instanceof Expression) returns without recording$model->geometries[$key], so deleting it would erase evidence of a real asymmetry. Recording it would currently be a no-op anyway, sinceperformInsert()'s restore loop would assign the same expression back. There is a genuine latent gap here — aSpatialExpressionassigned to a point attribute never becomes aPointafter save — but fixing that means recording the geometry behind the expression, which is a behaviour change and out of scope.Support/Utilsglobe ISO check — all 255 features in the bundledglobe.jsoncarry bothISO_A3andISO_A2(verified); the guard protects against future data.Api/v1/OrderController::capturePhoto's empty-photo check — defensive after avalidate()that already requires a non-emptyphotosarray.Api/v1/OrderController::updateActivity's not-found guard — blocked on an upstream defect:core-api'sModel::findByIdOrFail()calls a non-existentgetModelNotFoundException(), so it raisesBadMethodCallExceptioninstead ofModelNotFoundExceptionand a missing order returns 500 rather than 404. Remove the annotation oncecore-apiis patched. Details in this comment on #277.Coverage impact
The
--fail-under=100gate is still red: 165 statements remain, of which two are the annotated upstream-blocked lines and the rest are residue not yet classified.Verification
Run on the host runtime (asdf PHP 8.4 with Xdebug), no Docker:
composer test:lint— clean, 0 of 1140 files needing fixesXDEBUG_MODE=coverage COMPOSER_PROCESS_TIMEOUT=0 composer coverage:baseline— all suites greenphp scripts/coverage-summary.php coverage/clover.xml --fail-under=100Behavioural canaries specific to this change:
ZoneControllerBordersAndSeamsTest(4/4) andServiceAreaControllerBorderAndSeamsTest(3/3) snapshotted before and after, including a new polygon update round-trip that rewrites a border and reads the vertices back.Casts/Polygonshape, passes on the new one.coordsToCircle: asserted the ring still opens and closes at the same coordinate with no duplicated interior vertex.Three tests that asserted the old behaviour were updated (the Null Island lookup, and two that documented the
Casts/Polygonreturn shape). Each is called out in its diff with a comment explaining what changed and why.Notes for review
insertFromMixed(GoogleAddress), and two guards whose callee can in fact return null (Utils::getPointFromMixed()returnsnullfor an unresolvableplace_*/driver_*id — worth knowing, it is easy to assume otherwise from the signature).Casts/Pointreturns a rawPointfrom itsisCoordinatesarm while itsGeometryInterfacearm wraps — the same class of inconsistency as the Polygon fix, left alone to keep this diff's blast radius contained.