Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix fatal errors with deleteOptionsAndSelections #7711

Merged

Conversation

kassner
Copy link
Contributor

@kassner kassner commented Dec 7, 2016

Magento\BundleImportExport\Model\Import\Product\Type\Bundle::deleteOptionsAndSelections had no love during code convertion. Throws fatal errors because columns does not exist. Method refactored to work as expected.

@vrann vrann self-assigned this Mar 25, 2017
@vrann vrann added this to the March 2017 milestone Mar 25, 2017
@vrann
Copy link
Contributor

vrann commented Mar 25, 2017

@kassner unfortunately, we don't process PRs to 2.1 branch at the moment. Please re-create it against develop branch.

@vrann vrann closed this Mar 25, 2017
@kassner kassner changed the base branch from 2.1 to develop March 25, 2017 19:33
@kassner
Copy link
Contributor Author

kassner commented Mar 25, 2017

@vrann can you reopen? GitHub allows to rebase the PR from the interface.

@vrann vrann reopened this Mar 26, 2017
@vrann
Copy link
Contributor

vrann commented Mar 26, 2017

@kassner can you merge with the latest develop? There are conflicts right now.

@kassner kassner force-pushed the fix/feature-bundle-import-export branch from a506dca to 1886e26 Compare March 26, 2017 20:32
@kassner
Copy link
Contributor Author

kassner commented Mar 26, 2017

@vrann Done.

@vrann
Copy link
Contributor

vrann commented Apr 12, 2017

@kassner trying to reproduce the issue. Can you please provide steps and some sample data (i.e. file from which to import)

@kassner
Copy link
Contributor Author

kassner commented Apr 13, 2017

@vrann I can provide a descriptive way of reproducing it next week, but is quite easy to see the problem at naked eye: catalog_product_bundle_option doesn't have value_id or parent_product_id columns. Basically anything that calls deleteOptionsAndSelections will result in a fatal error.

@kassner
Copy link
Contributor Author

kassner commented Apr 13, 2017

@vrann Well, I can't provide a working example, because it looks like this part is never executed in normal conditions, since saveData is only called when saving a product (Add/Update or Replace behaviours), so it never goes inside that if, since it looks for the Delete behaviour.

Can, instead, I remove this code on this PR, since it isn't used at all? If not, just close this PR. We have this issue internally, but deleteOptionsAndSelections is called on replace (I can't tell the reasons, I didn't work on that part).

Thanks!

@vrann
Copy link
Contributor

vrann commented Apr 14, 2017

@kassner I see that the joined table catalog_product_bundle_option has column parent_id while catalog_product_bundle_option_value which is used in FROM part of the query has column value_id. So it should be possible to select value_id and filter by parent_id. Am I missing something?

I would keep that section because DELETE behavior can always be set to entity model before invocation of saveData()

@kassner
Copy link
Contributor Author

kassner commented Apr 18, 2017

@vrann The join is correct, but that is only used to populate $valueIds. The problem takes place when $this->connection->delete is called. Example:

$this->connection->delete(
$optionTable,
$this->connection->quoteInto('value_id IN (?)', array_keys($valuesIds))
);

This code is deleting from $optionTable (which resolves to catalog_product_bundle_option), but that table does not have value_id. In the end, all the 3 calls to $this->connection->delete are deleting from $optionTable instead of the correct ones.

@okorshenko okorshenko modified the milestones: April 2017, May 2017 May 9, 2017
@okorshenko okorshenko removed this from the May 2017 milestone Jun 1, 2017
@okorshenko okorshenko modified the milestones: June 2017, May 2017 Jun 1, 2017
@okorshenko okorshenko modified the milestones: June 2017, July 2017 Jul 2, 2017
@okorshenko okorshenko assigned okorshenko and unassigned vrann Jul 12, 2017
@okorshenko
Copy link
Contributor

Hi @kassner
If you would like to proceed with this PR, could you please add integration test to this file to cover your use case https://github.com/magento/magento2/blob/develop/dev/tests/integration/testsuite/Magento/BundleImportExport/Model/Import/Product/Type/BundleTest.php ?
Thank you
Please let me know if you need some help

@kassner
Copy link
Contributor Author

kassner commented Jul 13, 2017

@okorshenko I will not put more hours into this issue, up to you to merge or close.

@okorshenko okorshenko closed this Jul 13, 2017
@okorshenko okorshenko reopened this Jul 13, 2017
@okorshenko
Copy link
Contributor

Hi @kassner We will add tests on our side. Thank you for your contribution.

@magento-team magento-team merged commit 1886e26 into magento:develop Jul 18, 2017
magento-team pushed a commit that referenced this pull request Jul 18, 2017
[EngCom] Public Pull Requests

 - MAGETWO-70817: remove redundant else and use strict check #10271
 - MAGETWO-70787: update phpserver to support versioned static urls #10250
 - MAGETWO-70764: Fix overwrite default value image/file with NULL #10253
 - MAGETWO-70761: Show values that are duplicate in attribute error msg #10213
 - MAGETWO-70758: Fix for #4530 $product->getRatingSummary() always returns null […] #10248
 - MAGETWO-70706: simplify product lists #2 #9019
 - MAGETWO-70669: Fix fatal errors with deleteOptionsAndSelections #7711
 - MAGETWO-70464: Fix shipping address can use for billing #10130
 - MAGETWO-69913: magento/magento2 #9196 - Products ordered report doesn't show simple (child) products of configurable products #9908
@Ctucker9233
Copy link

@magento-team This PR Doesn't appear to be in 2.2. Should it be backported?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants