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

IBX-2597: Removing first Pesonalized block from page builder editor removes both previews #113

Conversation

GrabowskiM
Copy link
Contributor

@GrabowskiM GrabowskiM commented Apr 28, 2023

Question Answer
JIRA issue https://issues.ibexa.co/browse/IBX-2597
Type bug
Target Ibexa DXP version v3.3
BC breaks no
Doc needed yes

This bug occurs because normally encore_entry_link_tags is loaded only once per request, but we add {{ encore_entry_link_tags('ezplatform-personalization-css', null, 'ezplatform') }} in each block tempate. Combination of this results in situation, where CSS file is included only in first block so when we remove first block, including proper CSS disappears completely from page and styling for all other blocks is broken (non existing to be more specific).

This PR uses this approach https://github.com/symfony/webpack-encore-bundle/tree/v1.16.1#rendering-multiple-times-in-a-request-eg-to-generate-a-pdf to reset internal cache.

Please be gentle in CR (:P), solution is taken in 100% from this fragment where similar case occurs: https://github.com/ezsystems/ezplatform-admin-ui/blob/aa7aa34282f46f65a7300368f5e955d484a74404/src/lib/EventListener/AdminExceptionListener.php

Argument $fields in method ContentService::prepareFields expects array of string - field identifiers. This can be empty array but will never be a null.

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly (master for features, the oldest supported for bugs).
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review (ping @ezsystems/php-dev-team).

@ciastektk ciastektk requested a review from a team May 5, 2023 08:31
Copy link
Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

I just have a minor concern, because we're doing things slightly differently than the way it is shown in the example from Symfony.

Comment on lines +75 to +77
$this->encoreTagRenderer->reset();
$this->entrypointLookupCollection->getEntrypointLookup('ezplatform')->reset();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming that this is done inside a sub-request of sorts, and for whatever reason the scripts/links are not treated separately by Symfony, you should also reset the internal cache after the response is generated. It is done so in the example provided:

// src/Controller/SomeController.php

use Symfony\WebpackEncoreBundle\Asset\EntrypointLookupInterface;

class SomeController
{
    public function index(EntrypointLookupInterface $entrypointLookup)
    {
        $entrypointLookup->reset();
        // render a template

        $entrypointLookup->reset();  // <------ Note the second call
        // render another template

        // ...
    }
}

So what I think you want to do is store the response in a variable instead of returning it immediately, and call resets a second time. And only then return the response.

Without it, any scripts/links generated in the template will not be likewise present in the parent response. There is a practice where JS is included at the end of the HTML document, and that addresses it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added second call: a16f380

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 5, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@ciastektk ciastektk added bug Something isn't working Ready for QA labels May 5, 2023
@micszo micszo self-assigned this May 15, 2023
Copy link
Member

@micszo micszo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA Approved on Ibexa Commerce 3.3.33-dev, problem does not occur anymore.

@micszo micszo removed their assignment May 16, 2023
@ciastektk ciastektk merged commit 99498fa into 2.1 May 16, 2023
@ciastektk ciastektk deleted the IBX-2597-removing-first-pesonalized-block-from-page-builder-editor-removes-both-previews branch May 16, 2023 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working QA Approved
Development

Successfully merging this pull request may close these issues.

6 participants