Use product type id instead of product id

[tobias-forkel]

Description (*)

The condition supposed to match $product->getTypeId() against ['grouped', 'configurable', 'bundle'] - not the product id.

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

No testing required.

Questions or comments

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
• All automated tests passed successfully (all builds are green)

Resolved issues:

1. resolves [Issue] Use product type id instead of product id #35458: Use product type id instead of product id

[m2-assistant]

Hi @tobias-forkel. Thank you for your contribution
Here are some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

• @magento run all tests - run or re-run all required tests against the PR changes
• @magento run <test-build(s)> - run or re-run specific test build(s)
  For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE,
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests
13. Semantic Version Checker

You can find more information about the builds here

ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.

For more details, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, join the Community Contributions Triage session to discuss the appropriate ticket.

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

[manavluhar]

<?php
/**
 * Copyright © Magento, Inc. All rights reserved.
 * See COPYING.txt for license details.
 */
declare(strict_types=1);

namespace Magento\Catalog\Model\Product\Option;

use Magento\Catalog\Api\Data\ProductCustomOptionInterface;
use Magento\Catalog\Api\Data\ProductInterface;
use Magento\Catalog\Api\ProductCustomOptionRepositoryInterface as OptionRepository;
use Magento\Catalog\Model\Product\Option;
use Magento\Catalog\Model\ResourceModel\Product\Relation;
use Magento\Framework\App\ObjectManager;
use Magento\Framework\EntityManager\Operation\ExtensionInterface;
use Magento\Framework\Exception\CouldNotSaveException;

/**
 * SaveHandler for product option
 *
 * @SuppressWarnings(PHPMD.CouplingBetweenObjects)
 */
class SaveHandler implements ExtensionInterface
{
    /**
     * @var string[]
     */
    private array $compositeProductTypes = ['grouped', 'configurable', 'bundle'];

    /**
     * @var OptionRepository
     */
    protected OptionRepository $optionRepository;

    /**
     * @var Relation
     */
    private $relation;

    /**
     * @param OptionRepository $optionRepository
     * @param Relation|null $relation
     */
    public function __construct(
        OptionRepository $optionRepository,
        ?Relation        $relation = null
    ) {
        $this->optionRepository = $optionRepository;
        $this->relation = $relation ?: ObjectManager::getInstance()->get(Relation::class);
    }

    /**
     * Perform action on relation/extension attribute
     *
     * @param object $entity
     * @param array $arguments
     * @return ProductInterface|object
     * @SuppressWarnings(PHPMD.UnusedFormalParameter)
     * @throws CouldNotSaveException
     */
    public function execute($entity, $arguments = [])
    {
        if ($entity->getOptionsSaved()) {
            return $entity;
        }

        $options = $entity->getOptions();
        $optionIds = [];

        if ($options) {
            $optionIds = array_map(function (Option $option) {
                return $option->getOptionId();
            }, $options);
        }

        /** @var ProductInterface $entity */
        foreach ($this->optionRepository->getProductOptions($entity) as $option) {
            if (!in_array($option->getOptionId(), $optionIds)) {
                $this->optionRepository->delete($option);
            }
        }
        if ($options) {
            $this->processOptionsSaving($options, (bool)$entity->dataHasChangedFor('sku'), $entity);
        }

        return $entity;
    }

    /**
     * Save custom options
     * 
     * @param array $options
     * @param bool $hasChangedSku
     * @param ProductInterface $product
     * @return void
     * @throws CouldNotSaveException
     */
    private function processOptionsSaving(array $options, bool $hasChangedSku, ProductInterface $product): void
    {
        $isProductHasRelations = $this->isProductHasRelations($product);
        /** @var ProductCustomOptionInterface $option */
        foreach ($options as $option) {
            if (!$isProductHasRelations && $option->getIsRequire()) {
                $message = 'Required custom options can not be added to a simple product'
                    . ' that is a part of a composite product.';
                throw new CouldNotSaveException(__($message));
            }

            if ($hasChangedSku && $option->hasData('product_sku')) {
                $option->setProductSku($product->getSku());
            }
            $this->optionRepository->save($option);
        }
    }

    /**
     * Check if product doesn't belong to composite product
     *
     * @param ProductInterface $product
     * @return bool
     */
    private function isProductHasRelations(ProductInterface $product): bool
    {
        $result = true;
        if (!in_array($product->getTypeId(), $this->compositeProductTypes)
            && $this->relation->getRelationsByChildren([$product->getId()])
        ) {
            $result = false;
        }
        return $result;
    }
}

Can you please Update your PR according to the suggested changes? @tobias-forkel

[tobias-forkel]

@manavluhar

Thanks for your suggestion! I want to keep my first PR as simple as possible without adding further changes, especially wording. Create your own PR and explain why you want to change the spelling. Cannot and can not are both acceptable spellings in my opinion.

[manavluhar]

Hi @tobias-forkel , Except for this change(cannot to can not), I have suggested changes according to Coding Standards. If you review it once.

[manavluhar]

@tobias-forkel Can you please Sign the CLA so this PR can proceed further?

[sdzhepa]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

[tobias-forkel]

Thanks @manavluhar

I just signed the CLA and committed your suggestions.

[manavluhar]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

[Den4ik]

Hi @tobias-forkel
Please check failed static tests

[tobias-forkel]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

[engcom-Alfa]

Hi @tobias-forkel , @Den4ik & @manavluhar Thanks for your contribution and collaboration.
We agree that you have already mentioned there is no manual testing required on this PR file changes.
Still, we tried to analyse the code changes along with internal developer to identify any manual test scenario if we can find. But could not identify any scenario as such!

Can you please help us answering in brief, how the file changes are not affected any functional scenario?
So that, we can just re-think once and move the ticket forward with further process.

Thanks in advance!

[engcom-Alfa]

Hi @tobias-forkel ,
I tried to reach you thru Slack channel, but could not find you there!
You are invited to join the Magento Community Slack Channel, please feel free to join the workspace.

[hostep]

@engcom-Alfa, the mistake can be seen here, where getTypeId was mistakenly changed to getId by accident while refactoring: 1b7697f#diff-161b798378bbafeaa469c15a23f666c268a5bc6eb6be4df1584beb0d25223c20R124 (which is part of #31645)

What I think is the purpose of this in_array check, is to avoid having to call getRelationsByChildren and thus avoid executing unnecessary SQL queries when the product you are trying to create or update is of type grouped or configurable or bundle. Because those can have required custom options and can't belong to another composite product type.
In contrast, a simple or virtual product can belong to one of those 3 composite product types and if that is the case, it can't have a required custom option.
This is what the check is supposed to do I believe.

So steps to reproduce in the backoffice of Magento:

• Create a configurable product and create some children and give the configurable product a required custom option
• Create a simple product and give it a required custom option
• Change the code Magento\Catalog\Model\ResourceModel\Product\Relation::getRelationsByChildren and add this line to the top of the method:

\Magento\Framework\App\ObjectManager::getInstance()->get(\Psr\Log\LoggerInterface::class)->critical('getRelationsByChildren called!');

• Now edit the configurable product and save it, expected is that there is no output getRelationsByChildren called! in the system.log file
• Now edit the simple product and save it, expected is that there is output getRelationsByChildren called! in the system.log file
• Open one of the simple/virtual products that belongs to the configurable product, try to give it a required custom option and save it, expected is that there is output getRelationsByChildren called! and there is an error when trying to save the product saying: Required custom options cannot be added to a simple product that is a part of a composite product.

@tobias-forkel: correct me if I'm wrong 🙂

[engcom-Alfa]

Hi @hostep Thank you so much for the detailed above explanation!

I have followed to identify the issue as mentioned below that just follows as you explained above

1. Created a configurable product and with the custom options as shown in the screenshot:

2. Created a simple product with custom options too as shown in the below screenshot:

3. Updated one line of code in the file & function: Magento\Catalog\Model\ResourceModel\Product\Relation::getRelationsByChildren ; The added line as first line of code inside the function is \Magento\Framework\App\ObjectManager::getInstance()->get(\Psr\Log\LoggerInterface::class)->critical('getRelationsByChildren called!');. The updated line is shown in the below screenshot.

4. Edited the configurable product & saved. Checked the system.log file latest logs, and found the log text getRelationsByChildren called! [] []

5. Edited the simple product also, and found the similar log updated in the file mentioned in step4.

6. Opened the configurable product's compounded simple product with the colour option to edit it, and added the new custom option to it. Saved the product.

   • Checked the system.log file latest logs, and found the log text getRelationsByChildren called! [] []
   • Also got the error Required custom options cannot be added to a simple product that is a part of a composite product. in the UI.
   • The same exception message was available in the system.log file also.

Query: I took the file changes into the local and repeated the above step6. Didn't notice any change in the feature after pulling PR code changes. So, can you please help me to understand in which scenario the file changes are going to be utilised and what fix does it do?
@tobias-forkel

@hostep Thanks for this additional info about this PR #31645 too!

[hostep]

@engcom-Alfa, you should repeat from step 4 with the code changes from this PR, in step 4 you should no longer see getRelationsByChildren called! in the system.log file. All the rest should act the same as before.

To repeat myself, the point of this change, is that a certain SQL statement is no longer executed when saving configurable, bundled and grouped products when they have a required custom option. Which is a micro optimisation. But it's a good one because the code as it is currently makes no sense (even though it doesn't really cause issues).

[engcom-Alfa]

@engcom-Alfa, you should repeat from step 4 with the code changes from this PR, in step 4 you should no longer see getRelationsByChildren called! in the system.log file. All the rest should act the same as before.

To repeat myself, the point of this change, is that a certain SQL statement is no longer executed when saving configurable, bundled and grouped products when they have a required custom option. Which is a micro optimisation. But it's a good one because the code as it is currently makes no sense (even though it doesn't really cause issues).

Thank you for the reply!
I can proceed ahead.
Thanks much :)

[engcom-Alfa]

@magento create issue

[engcom-Alfa]

✔️ QA Passed

Manual testing scenario:

1. Execute step by step till step 5 given in the above-comment.

Before: ✖️ Updating configurable products functionality was calling a method getRelationsByChildren, that gets logged in system.log file

After: ✔️ Updating configurable products functionality not calling a method getRelationsByChildren, that gets logged in system.log file as like simple product update

It is a minor code fix and has no impact on functional features explicitly, required no additional regression test cases.

[tobias-forkel]

Well explained @hostep! Thank you!

Yes, depending on how product options are setup, the backend will falsely return Required custom options cannot be added to a simple product that is a part of a composite product. while saving the product - because of if (!in_array($product->getId(), $this->compositeProductTypes).

In this particular case I was able to fix it by manually changing the condition in vendor/magento/module-catalog/Model/Product/Option/SaveHandler.php and save the configurable product again. I did not investigate further - but I noticed the mistake in the condition ( $product->getId() instead of $product->getTypeId() ) and decided to open a PR.