Steps to reproduce I assume would be something like this:

- Create some attribute entities
- Create a product with a variation that references one of the attributes.
- Delete the attribute
- Go back to the variations tab of the product that now has an invalid value

The function getAttributeValues assumes we are able to extract entities when a field is not empty. That is not always the case, for example if the field is not empty, but the target id value refers to a deleted attribute.

That would in turn result in a stack trace like the following (here for googlability):

Error: Call to a member function hasTranslation() on null in Drupal\commerce\Entity\CommerceContentEntityBase->ensureTranslations() (line 48 of /var/www/html/drupal/modules/contrib/commerce/src/Entity/CommerceContentEntityBase.php) #0 /var/www/html/drupal/modules/contrib/commerce/modules/product/src/Entity/ProductVariation.php(349): Drupal\commerce\Entity\CommerceContentEntityBase->ensureTranslations(Array)
#1 /var/www/html/drupal/modules/contrib/commerce/modules/product/src/ProductVariationListBuilder.php(123): Drupal\commerce_product\Entity\ProductVariation->getAttributeValues()
#2 /var/www/html/drupal/modules/contrib/commerce/modules/product/src/ProductVariationListBuilder.php(191): Drupal\commerce_product\ProductVariationListBuilder->buildRow(Object(Drupal\commerce_product\Entity\ProductVariation))
#3 [internal function]: Drupal\commerce_product\ProductVariationListBuilder->buildForm(Array, Object(Drupal\Core\Form\FormState))
#4 /var/www/html/drupal/core/lib/Drupal/Core/Form/FormBuilder.php(519): call_user_func_array(Array, Array)
#5 /var/www/html/drupal/core/lib/Drupal/Core/Form/FormBuilder.php(276): Drupal\Core\Form\FormBuilder->retrieveForm('commerce_produc...', Object(Drupal\Core\Form\FormState))
#6 /var/www/html/drupal/core/lib/Drupal/Core/Form/FormBuilder.php(217): Drupal\Core\Form\FormBuilder->buildForm(Object(Drupal\commerce_product\ProductVariationListBuilder), Object(Drupal\Core\Form\FormState))
#7 /var/www/html/drupal/modules/contrib/commerce/modules/product/src/ProductVariationListBuilder.php(156): Drupal\Core\Form\FormBuilder->getForm(Object(Drupal\commerce_product\ProductVariationListBuilder))
#8 /var/www/html/drupal/core/lib/Drupal/Core/Entity/Controller/EntityListController.php(23): Drupal\commerce_product\ProductVariationListBuilder->render()
#9 [internal function]: Drupal\Core\Entity\Controller\EntityListController->listing('commerce_produc...')
#10 /var/www/html/drupal/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(123): call_user_func_array(Array, Array)
#11 /var/www/html/drupal/core/lib/Drupal/Core/Render/Renderer.php(582): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
#12 /var/www/html/drupal/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(124): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure))
#13 /var/www/html/drupal/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(97): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array)
#14 /var/www/html/vendor/symfony/http-kernel/HttpKernel.php(151): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
#15 /var/www/html/vendor/symfony/http-kernel/HttpKernel.php(68): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1)
#16 /var/www/html/drupal/core/lib/Drupal/Core/StackMiddleware/Session.php(57): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#17 /var/www/html/drupal/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(47): Drupal\Core\StackMiddleware\Session->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#18 /var/www/html/drupal/core/modules/page_cache/src/StackMiddleware/PageCache.php(106): Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#19 /var/www/html/drupal/core/modules/page_cache/src/StackMiddleware/PageCache.php(85): Drupal\page_cache\StackMiddleware\PageCache->pass(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#20 /var/www/html/drupal/modules/contrib/shield/src/ShieldMiddleware.php(84): Drupal\page_cache\StackMiddleware\PageCache->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#21 /var/www/html/drupal/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(47): Drupal\shield\ShieldMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#22 /var/www/html/drupal/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(52): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#23 /var/www/html/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#24 /var/www/html/drupal/core/lib/Drupal/Core/DrupalKernel.php(693): Stack\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#25 /var/www/html/drupal/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request))
#26 {main}.
CommentFileSizeAuthor
#10 3076336-10.patch1.66 KBeiriksm
#10 3076336-10-test-only.patch1.04 KBeiriksm
#4 3076336-4-test-only.patch2.83 KBeiriksm
#4 3076336-4.patch3.46 KBeiriksm
#2 3076336.patch644 byteseiriksm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

eiriksm created an issue. See original summary.

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS, Violinist, Foreningen Drupal Norge commented
Status: Active » Needs review
FileSize
3076336.patch644 bytes

Should also be fairly easy to write a test for, but here is a patch for now.

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS, Violinist, Foreningen Drupal Norge commented
Issue summary: View changes

Just updating with steps to reproduce.

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS, Violinist, Foreningen Drupal Norge commented
FileSize
3076336-4.patch3.46 KB
3076336-4-test-only.patch2.83 KB

...and here is one with a test, and a test-only patch that should fail.

Status: Needs review » Needs work

The last submitted patch, 4: 3076336-4-test-only.patch, failed testing. View results

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS, Violinist, Foreningen Drupal Norge commented
Status: Needs work » Needs review

..back to needs review since the test fail was very expected :)

djg_tram’s picture

djg_tram CreditAttribution: djg_tram commented

I'm not saying having a test is a Bad Thing but the bug is out there, unfixed. The patch is a single line, a very obvious nullcheck, and such an obvious nullcheck can't cause any regression issues for sure. And in fact, adding it fixes the bug. So, it should be released ASAP and not waiting for some tests to arrive and doing nothing in the meantime. :-)

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
Status: Needs review » Needs work

The bug(fix) is in ProductVariation::getAttributeValues(). So the test should be in the ProductVariationTest kernel test.

Right now we're extending the product attributes functional test, but it is relying on an implementation detail, that the variations listing somehow results in getAttributeValues() being called. If that logic changes in the future (as is planned), then the test ends up covering nothing.

Also, the current test doesn't reference the attribute value by ID, so the reference is in fact always broken.

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented

Added missing attribute test coverage to ProductVariationTest in #3087350: The product variation kernel test is missing coverage.

The assertions needed by this issue should now only be a few lines, at the end of testAttributes().
I suggest setting back the size attribute, then deleting the size attribute, then confirming getAttributeValues / getAttributeValue output.

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS, Violinist, Foreningen Drupal Norge commented
Status: Needs work » Needs review
FileSize
3076336-10-test-only.patch1.04 KB
3076336-10.patch1.66 KB

Very nice!

Here is an updated patch.

The last submitted patch, 10: 3076336-10-test-only.patch, failed testing. View results

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented

Thanks!
One last review. I'll tweak this pre-commit.

// Make sure things work as expected when a variation references a deleted
    // attribute.

I like to use "Confirm that..." language in test comments.
In this case I'd go with "Confirm that deleted attribute values are handled properly." since it fits in one line.

+    // @see https://www.drupal.org/project/commerce/issues/3076336

We don't reference Drupal issues in code. That's what git blame is for :)

We're also missing coverage for the singular getAttributeValue().

  • bojanz committed 2becf2c on 8.x-2.x authored by eiriksm 
    Issue #3076336 by eiriksm, bojanz: If a variation references a deleted...
bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
Status: Needs review » Fixed

Committed. Thanks!

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
Parent issue: » #2898215: [meta] Handle deletion of Commerce entities that are in use

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.