Fix fatal error on line 427, as method hasWarranty expects product id instead of sku #426
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ViewModel/Warranty.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ViewModel/Warranty.php (2)
Observer/AddToCart.php (1)
getQuote(81-84)Plugin/Checkout/CustomerData/AbstractItemPlugin.php (1)
hasWarranty(118-121)
ViewModel/Warranty.php
Outdated
| * @param int $productId | ||
| * @return bool | ||
| */ | ||
| public function isWarrantyInQuote(string $sku): bool | ||
| public function isWarrantyInQuote(int $productId): bool | ||
| { | ||
| try { | ||
| $quote = $this->checkoutSession->getQuote(); | ||
| } catch (LocalizedException $exception) { | ||
| $quote = null; | ||
| } | ||
| if ($quote) { | ||
| $hasWarranty = $this->hasWarranty($quote, $sku); | ||
| $hasWarranty = $this->hasWarranty($quote, $productId); | ||
| } | ||
| return $hasWarranty ?? false; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify parameter semantics: quote item ID vs product ID.
While the type change from string to int fixes the immediate fatal error, there's a potential semantic issue:
- The parameter is now named
$productId(line 419) - However,
hasWarranty()calls$quote->getItemById($id)(line 229), which expects a quote item ID, not a product ID - In Magento, these are distinct: a quote item ID identifies an item in the cart, while a product ID identifies a catalog product entity
Additionally, this is a breaking API change to a public method marked for backwards compatibility with the Hyva module (line 414). External callers using the old signature isWarrantyInQuote(string $sku) will break.
Please verify:
- How is this method currently being called? Are callers passing quote item IDs or product IDs?
- Should the parameter be renamed to
$quoteItemIdfor clarity? - Has the Hyva module integration been updated to match this signature change?
🏁 Script executed:
#!/bin/bash
# Search for calls to isWarrantyInQuote to understand how it's being used
rg -nP --type=php -C3 '\bisWarrantyInQuote\s*\('Length of output: 382
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for hasWarranty method definition and calls to understand what ID type it expects
rg -nP --type=php -C5 'hasWarranty\s*\('
Length of output: 5727
Critical logic error: parameter type fixed but semantic meaning incorrect.
The PR fixed the type error (string → int), but introduced a logic bug:
hasWarranty()at line 227 expects a quote item ID: it calls$quote->getItemById($id)(line 229)- All internal calls to
hasWarranty()pass quote item IDs from$_item->getId()(see warranty.phtml:19, warranty-offers.phtml:39) - But
isWarrantyInQuote()passes$productId(line 427), which is a product catalog ID, not a quote item ID
These are semantically different values in Magento. Passing a product ID to getItemById() will not correctly locate the quote item, breaking method functionality.
The method needs to iterate through quote items to find the one matching the product ID, then check that item for warranty status—not pass the product ID directly to hasWarranty().
🤖 Prompt for AI Agents
In ViewModel/Warranty.php around lines 416-430, isWarrantyInQuote() incorrectly
passes a product catalog ID to hasWarranty(), which expects a quote item ID;
update the method to retrieve the current quote (catching LocalizedException as
before), iterate the quote items to find the item whose getProductId() matches
the given $productId, then call hasWarranty() with that quote item's ID (or
return false if no matching item found). Ensure you handle the case of no quote
or no matching item and return false.
There was a problem hiding this comment.
renamed into $quoteItemId
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current implementation of isWarrantyInQuote method
rg -nP --type=php -A 15 'public function isWarrantyInQuote\(' ViewModel/Warranty.php
Length of output: 513
🏁 Script executed:
#!/bin/bash
# Check the docblock for isWarrantyInQuote method
rg -nP --type=php -B 8 'public function isWarrantyInQuote\(' ViewModel/Warranty.php
Length of output: 285
@sergeynezbritskiy Perfect! ✅ The rename to $quoteItemId correctly resolves the semantic issue I flagged. The parameter name now accurately reflects that the method expects a quote item ID (used in getItemById() within hasWarranty()), not a product catalog ID.
Verified changes:
- Method signature (line 419):
public function isWarrantyInQuote(int $quoteItemId): bool - Docblock (line 416):
@param int $quoteItemId - Internal call (line 427):
$hasWarranty = $this->hasWarranty($quote, $quoteItemId);
Important reminder: This is still a breaking API change from the original isWarrantyInQuote(string $sku) signature. Since the docblock notes this method is "kept for backwards compatibility with Hyva module" (line 414), please ensure the Hyva module integration is updated to pass the quote item ID as an integer instead of the SKU as a string.
🧬 Code Graph Analysis Results
<ViewModel/Warranty.php snippet>
- File: ViewModel/Warranty.php
- Lines: 170-202
public function isWarrantyInQuote(int $quoteItemId): bool
{
try {
$quote = $this->checkoutSession->getQuote();
} catch (LocalizedException $exception) {
$quote = null;
}
if ($quote) {
$hasWarranty = $this->hasWarranty($quote, $quoteItemId);
}
return $hasWarranty ?? false;
}
</ViewModel/Warranty.php snippet>
1 participant
There is a fatal error in the module
main.CRITICAL: TypeError: Extend\Warranty\ViewModel\Warranty::hasWarranty(): Argument #2 ($id) must be of type int, string given, called in .../vendor/extend/module-warranty/ViewModel/Warranty.php on line 427 and defined in .../vendor/extend/module-warranty/ViewModel/Warranty.php:227
Summary by CodeRabbit