DGS-448 Guard pickup point lookup miss in checkout parcel shop selection - #184
Draft
TLabutis wants to merge 1 commit into
Draft
DGS-448 Guard pickup point lookup miss in checkout parcel shop selection#184TLabutis wants to merge 1 commit into
TLabutis wants to merge 1 commit into
Conversation
When the city and street posted from checkout match no row in dpd_shop, ParcelShopRepository::getIdByCityAndStreet returns false. That false was passed straight through DPDShop::getShopByPudoId, whose getFirst() also returns false, so $pudo->longitude and $pudo->latitude resolved to null. getClosestPudoShops then interpolated pSQL(null) into the haversine query, producing sin(( * pi() / 180)) and a SQL syntax error. executeS returned false, and ShopFactory::createShop is type-hinted array, so the request died with a TypeError and a 500. The failure was silent rather than merely broken because all three error handlers in pudo-search.js called DPDdisplayMessage($container, ...) while $container was declared in only one of the three functions, and even there from $(this).closest(), which resolves to an empty set in a plain function call. The other two threw a ReferenceError. Each handler also parsed response.responseText unguarded, and a PHP fatal returns an HTML error page, so the parse threw first. The customer saw an empty map and no message at all. PudoService::getClosestParcelShops now returns an empty array when the pudo is not a loaded object or the query fails, which removes the fatal. saveParcelShop turns the empty result into a status:false payload with a translated message, matching the !$isSuccess guard directly above it. Each error handler now scopes its own container lookup at response time, matching what the success handlers already did, and guards the parse. Also reordered the GoogleApiService constructor so isSslEnabled is assigned before getGeolocationUrl reads it, and dropped the branch that appended a second "s" to an already-complete "https" scheme. These cancelled each other out, so shipped output was always a correct https:// and this change is behaviour-neutral. Reordering alone would have introduced httpss:// on every SSL-everywhere shop, hence both. Not addressed: why the city and street data diverged after the 3.3.1 upgrade. This is a defensive fix, not a confirmed root-cause fix. The rewritten locker fetch path is the prime suspect but confirming it needs merchant environment details that are not in the ticket. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GUhfYfmoGVzjN1xL7S2Am8
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.
Problem
Merchants reported the checkout map showing no lockers after updating to 3.3.1, having already verified that the Google API key is active, the city dropdown populates, and lockers are present in the database.
Those three checks pass because the map never receives locker data as JSON.
DPDinitMarkers()inviews/js/front/pudo.jsscrapes the rendered DOM, readinginput[name="pudo-lat"]andinput[name="pudo-lng"]out of.dpd-services-block .list-group-item. Anything that stops those rows from rendering produces exactly the reported symptom set: list and dropdown look fine, server-side data is fine, map is empty.Root cause of the 500
When the city and street posted from checkout match no row in
dpd_shop:ParcelShopRepository::getIdByCityAndStreet()returnsDb::getValue()=false.DPDShop::getShopByPudoId(false)returnsPrestaShopCollection::getFirst()=false.$pudo->longitude/$pudo->latitudeonfalseresolve tonull.getClosestPudoShops(null, null, ...)interpolatespSQL(null)=''into the haversine query, producingsin(( * pi() / 180)), a SQL syntax error.Db::executeS()returnsfalse.ShopFactory::createShop(array $shops)is type-hinted, sofalsethrowsTypeError. Uncaught, HTTP 500.Reproduced step by step in isolation on PHP 7.2 and 8.5.
Why it was silent
All three
error:handlers inviews/js/front/pudo-search.jscalledDPDdisplayMessage($container, ...), but$containerwas declared in only one of the three functions, and even there as$(this).closest('.dpd-pudo-container'), which resolves to an empty jQuery set becausethisiswindowin a plain function call. In the other two it was never declared at all, so they threw aReferenceError.Each handler also called
JSON.parse(response.responseText)unguarded. A PHP fatal returns an HTML error page, so the parse threw before theReferenceErrorwas even reached. The customer saw an empty map and no message.Changes
PudoService::getClosestParcelShops()returns[]when the pudo is not a loaded object or the query failed. This removes the fatal.saveParcelShop()guards!isset($parcelShops[0])and returns astatus: falsepayload with a translated message, matching the!$isSuccessguard directly above it. The empty result becomes a visible message instead of a silent empty map.error:handler scopes its own container lookup at response time, matching what the success handlers already did, and wraps the parse in try/catch with aresponseText.templatepresence check.Included but not part of the fix
The
GoogleApiServicehunks are behaviour-neutral hardening, not a fix, and can be dropped from this PR if preferred.getGeolocationUrl()built$url = 'https'; if ($this->isSslEnabled) { $url .= 's'; }, which would yieldhttpss://. But the constructor called it before assigning$this->isSslEnabled, and the property has no default, so it was alwaysnullat call time and the branch never fired. Two defects that cancelled out. Verified on PHP 5.6 and 8.5:The middle column is the reason both were changed together: fixing the constructor ordering alone would have introduced a live
httpss://break on every SSL-everywhere shop.Testing
php -lon all 3 changed PHP files at 5.6 / 7.2 / 7.4 / 8.4 / 8.5node --check views/js/front/pudo-search.jsTypeErrorconfirmedPHP floor taken from
composer.json("php": "5.6"platform,">=5.6"require) andps_versions_compliancymin 1.7.1.0, not assumed.The
tests/Unitsuite was not run. It is not currently runnable, which is pre-existing and unrelated to this change: the tests extendPHPUnit_Framework_TestCase, removed in PHPUnit 8 which is the bundledphpunit.pharversion, andbootstrap.phploads onlyvendor/autoload.phpwhile the tests instantiateLanguage,ShopandConfiguration. There is no Makefile or CI config in the repo. Separately,GoogleApiServiceTest.php, the only test touching a changed file, makes live Google Maps API calls and contains no assertions.Scope limit
This is a defensive fix, not a confirmed root-cause fix. It makes the path unable to fatal and makes it report instead of failing silently. Why the city and street data stopped resolving after the upgrade is not established. The 3.3.1 locker fetch rewrite (
ExtendedApiClient,ExtendedParcelShopSearchFactory,FastParcelShopResponseParser,FastParcelShopSearch) repopulatesdpd_shopand is the prime suspect, but confirming it needs the merchant PS version, module version, shop URL and browser console output, none of which are in the ticket.Changelog entry added under
## [3.3.2].$this->versionindpdbaltics.phpis deliberately left at 3.3.1, since the bump belongs to the release commit and DGS-442 also targets 3.3.2.Ticket: https://invertus.atlassian.net/browse/DGS-448
🤖 Generated with Claude Code
https://claude.ai/code/session_01GUhfYfmoGVzjN1xL7S2Am8