Do not call html() on an empty carrier extra content collection - #215
Open
ShaiMagal wants to merge 1 commit into
Open
Do not call html() on an empty carrier extra content collection#215ShaiMagal wants to merge 1 commit into
ShaiMagal wants to merge 1 commit into
Conversation
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.
updatedDeliveryFormcalls.html()on the result of.next('.carrier-extra-content'). When that selector matches nothing, jQuery returns an empty collection,.html()returnsundefinedand.trim()throws on every carrier change. This adds the missing length check.Why
Nothing guarantees that
.carrier-extra-contentstays the immediate next sibling of.delivery-option. Any element inserted in between makes.next()return an empty collection, which is a common situation once a pickup point module places its selector next to the carrier row.The exception is also hard to diagnose, because jQuery 3 converts it into a rejection and the
.fail()branch inthemes/_core/js/checkout-delivery.jsthen callsprestashop.trigger(...), a method the emitter does not have. The console only showsr(...).trigger is not a function, and the real cause stays hidden. That second problem lives in the main repository and is not addressed here.The guard being fixed was introduced in 2.1.x by acd52d3 (#157) to solve PrestaShop/PrestaShop#37194, and this change keeps that behaviour: the extra content is still only slid down when it actually has content.
The
developbranch is not affected, it callsslideDown()directly and jQuery ignores an empty set there.How to test
.carrier-extra-contentblock.$('.delivery-option').first().after('<div></div>');in the console to simulate a module inserting an element between the carrier row and its extra content.TypeError. After it, the carrier switch is silent and the extra content of the selected carrier still expands when it has content.