productDetailViews event is fired very rarely, because hook_entity_view is invoked only once after each cache rebuild.
Is it expected behavior?
I assume productDetailViews is expected to be fired on every product details page load.

CommentFileSizeAuthor
#11 interdiff-3074305-08-11.txt9.15 KBwengerk
#11 3074305-11.patch8.41 KBwengerk
#11 3074305-11--should-fail.patch2.26 KBwengerk
#8 3074305-8.patch4.84 KBzeuty
#6 3074305-4.patch4.67 KBzeuty
#3 3074305-3.patch4.88 KBzeuty
#2 3074305-2.patch4.19 KBzeuty

Comments

zeuty created an issue. See original summary.

zeuty’s picture

zeuty
he/him
Primary language Belarusian
Location Tilburg
commented
StatusFileSize
new 3074305-2.patch4.19 KB

Patch that fixes the issue by moving productDetailsView event generation from hook_entity_view to Kernel Request Event.

zeuty’s picture

zeuty
he/him
Primary language Belarusian
Location Tilburg
commented
StatusFileSize
new 3074305-3.patch4.88 KB
wengerk’s picture

wengerk
he/him
Primary language French
Location Lausanne
commented
Status: Active » Needs work
wengerk’s picture

wengerk
he/him
Primary language French
Location Lausanne
commented

Hey Zeuty, Thanks for helping us on this issue !!

Do you need any help to update the tests ?

zeuty’s picture

zeuty
he/him
Primary language Belarusian
Location Tilburg
commented
StatusFileSize
new 3074305-4.patch4.67 KB
zeuty’s picture

zeuty
he/him
Primary language Belarusian
Location Tilburg
commented
Status: Needs work » Needs review

Please review the patch.
Functionality change:
Earlier productDetailViews was fired in commerce_google_tag_manager_commerce_product_view (hook_entity_view implementation), which is not the right one for the event, as it is not executed on every product details page load.
In patch productDetailViews execution is moved to KernelEvents::REQUEST event subscriber, so we can guarantee that it is fired during every product details page load.

zeuty’s picture

zeuty
he/him
Primary language Belarusian
Location Tilburg
commented
StatusFileSize
new 3074305-8.patch4.84 KB

Added some comments to clarify why priority 28 is used.

wengerk’s picture

wengerk
he/him
Primary language French
Location Lausanne
commented

Many thanks for your help !

I think, we should add a tests that show why the previous code didn't now works.
I mean:

  1. Create a tests that will fail using the old code & upload this here to ensure it fails properly as expected
  2. Porting this new tests in your patch and show it does not fails

Once done, we would be sure we close an important issue & we cover any regression on this.
What do you think ?

zeuty’s picture

zeuty
he/him
Primary language Belarusian
Location Tilburg
commented

@Wengerk, agreed, I will work on this.

wengerk’s picture

wengerk
he/him
Primary language French
Location Lausanne
commented
StatusFileSize
new 3074305-11--should-fail.patch2.26 KB
new 3074305-11.patch8.41 KB
new interdiff-3074305-08-11.txt9.15 KB

Hey guys, happy to finally got times get working on this issue !!

So here is a patch reroll, containing improvements and tests coverage !

I also upload a "should-fail" patch to ensure the bug has been covered.
Let's test all of that via testbot !

wengerk’s picture

wengerk
he/him
Primary language French
Location Lausanne
commented
Status: Needs review » Reviewed & tested by the community
wengerk’s picture

wengerk
he/him
Primary language French
Location Lausanne
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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