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

Set twig globals after first admin instanciation #8125

Merged
merged 1 commit into from
Nov 20, 2023

Conversation

AntoineRoue
Copy link
Contributor

Fixes #8120

Changelog

### Fixed
- Needing to submit form twice when using CRUDController::render()

@AntoineRoue AntoineRoue force-pushed the twig-globals branch 4 times, most recently from b4e52bd to 7954685 Compare November 19, 2023 21:18
@AntoineRoue
Copy link
Contributor Author

I should have converted it to draft but did not find the button. It's done now.

@core23 core23 marked this pull request as draft November 20, 2023 07:36
@VincentLanglet VincentLanglet marked this pull request as ready for review November 20, 2023 08:59
@VincentLanglet
Copy link
Member

Thanks !

@VincentLanglet VincentLanglet merged commit aab78ff into sonata-project:4.x Nov 20, 2023
24 of 25 checks passed
@core23
Copy link
Member

core23 commented Nov 24, 2023

Sorry for the late feedback. This solution does only work if your Controller extends the CRUDController.

If you want to create small (batch) action controller, there is no need to extend that controller and this solution does not work.

@AntoineRoue AntoineRoue deleted the twig-globals branch November 24, 2023 18:26
@AntoineRoue
Copy link
Contributor Author

If you don't extend CRUDController, then do you really need 'admin' and 'base_template' variables in twig ?

Before these globals were set, if extended CRUDController then we had to use CRUDController::renderWithExtraParams to render the template. Without extending CRUDController, if we wanted these variable in twig, we had to add them ourself when rendering the template. In the end, it would work like before no ?

@Warxcell
Copy link
Contributor

Unfortunately this breaks when inside long-running process like roadrunner. It throws exception that admin already exists as global, because it was set by previous admin.

@VincentLanglet
Copy link
Member

Unfortunately this breaks when inside long-running process like roadrunner. It throws exception that admin already exists as global, because it was set by previous admin.

Hi, can you try the 4.29.1 version ?

@Warxcell
Copy link
Contributor

Unfortunately this breaks when inside long-running process like roadrunner. It throws exception that admin already exists as global, because it was set by previous admin.

Hi, can you try the 4.29.1 version ?

I saw the fix, but isn't that just hiding the error? Seems like the global twig variable admin will stay forever as initially set, and all furhter reads will read the first admin instance.

@VincentLanglet
Copy link
Member

I saw the fix, but isn't that just hiding the error? Seems like the global twig variable admin will stay forever as initially set, and all furhter reads will read the first admin instance.

I'm not sure about this since the code allows to update globals:

/**
     * Registers a Global.
     *
     * New globals can be added before compiling or rendering a template;
     * but after, you can only update existing globals.
     *
     * @param mixed $value The global value
     */
    public function addGlobal(string $name, $value)
    {
        if ($this->extensionSet->isInitialized() && !\array_key_exists($name, $this->getGlobals())) {
            throw new \LogicException(sprintf('Unable to add global "%s" as the runtime or the extensions have already been initialized.', $name));
        }

        if (null !== $this->resolvedGlobals) {
            $this->resolvedGlobals[$name] = $value;
        } else {
            $this->globals[$name] = $value;
        }
    }

@Warxcell
Copy link
Contributor

I saw the fix, but isn't that just hiding the error? Seems like the global twig variable admin will stay forever as initially set, and all furhter reads will read the first admin instance.

I'm not sure about this since the code allows to update globals:

/**
     * Registers a Global.
     *
     * New globals can be added before compiling or rendering a template;
     * but after, you can only update existing globals.
     *
     * @param mixed $value The global value
     */
    public function addGlobal(string $name, $value)
    {
        if ($this->extensionSet->isInitialized() && !\array_key_exists($name, $this->getGlobals())) {
            throw new \LogicException(sprintf('Unable to add global "%s" as the runtime or the extensions have already been initialized.', $name));
        }

        if (null !== $this->resolvedGlobals) {
            $this->resolvedGlobals[$name] = $value;
        } else {
            $this->globals[$name] = $value;
        }
    }

Yes, but only if no exception is thrown? In our case - exception IS thrown.

@VincentLanglet
Copy link
Member

Yes, but only if no exception is thrown? In our case - exception IS thrown.

According to if ($this->extensionSet->isInitialized() && !\array_key_exists($name, $this->getGlobals())) {

Exception is thrown only if the global doesn't exists and the extension is initialized.
So no exception is thrown in your example.

Let's end this theoretically discussion here and wait for a well-reported bug instead if it exists.

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.

Submitting a form with ->render instead of ->renderWithExtraParams does not work
4 participants