NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[TwigBridge] Fix Bootstrap 4 form error layout#52373

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

asispts wants to merge4 commits intosymfony:5.4

base:5.4

Choose a base branch

frompts-contrib:fix/bootstrap-4-form-layout

Open

[TwigBridge] Fix Bootstrap 4 form error layout#52373

asispts wants to merge4 commits intosymfony:5.4frompts-contrib:fix/bootstrap-4-form-layout

Conversation

Copy link

Contributor

asispts commentedOct 31, 2023

Q	A
Branch?	5.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	Fix#52352
License	MIT

This PR will fix Bootstrap 4 form layout when using a required marker.

Fix bootstrap 4 form error layout

8dcb1df

carsonbot added Status: Needs Review Bug labels

Oct 31, 2023

carsonbot added this to the5.4 milestone

Oct 31, 2023

Copy link

carsonbot commentedOct 31, 2023

It looks like you unchecked the "Allow edits from maintainer" box. That is fine, but please note that if you have multiple commits, you'll need to squash your commits into one before this can be merged. Or, you can check the "Allow edits from maintainers" box and the maintainer can squash for you.

Cheers!

Carsonbot

Fix tests

7ff0b72

asispts commented

Oct 31, 2023

View reviewed changes

src/Symfony/Bridge/Twig/Tests/Extension/AbstractBootstrap4HorizontalLayoutTestCase.php OutdatedShow resolvedHide resolved

Copy link

ContributorAuthor

asispts commentedOct 31, 2023•
edited
Loading

Here is the result.

nicolas-grekas added the Form label

Oct 31, 2023

carsonbot changed the title~~Fix bootstrap 4 form error layout~~[Form] Fix bootstrap 4 form error layout

Oct 31, 2023

nicolas-grekas added TwigBridge and removed Form labels

Oct 31, 2023

carsonbot changed the title~~[Form] Fix bootstrap 4 form error layout~~[TwigBridge] Fix bootstrap 4 form error layout

Oct 31, 2023

OskarStark reviewed

Oct 31, 2023

View reviewed changes

src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_4_layout.html.twigShow resolvedHide resolved

smnandre reviewed

Oct 31, 2023

View reviewed changes

src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_4_layout.html.twig OutdatedShow resolvedHide resolved

Revert div

d9d92e6

Copy link

ContributorAuthor

asispts commentedNov 2, 2023

I think this failure is related.

1) Symfony\Bridge\Twig\Tests\Node\FormThemeTest::testCompileFailed asserting that two strings are equal.--- Expected+++ Actual@@ @@-'$this->env->getRuntime("Symfony\\Component\\Form\\FormRenderer")->setTheme(($context["form"] ?? null), [0 => "tpl1", 1 => "tpl2"], true);'+'$this->env->getRuntime("Symfony\\Component\\Form\\FormRenderer")->setTheme(($context["form"] ?? null), ["tpl1", "tpl2"], true);'/home/runner/work/symfony/symfony/src/Symfony/Bridge/Twig/Tests/Node/FormThemeTest.php:63

Any idea how to fix the test?

symfony/src/Symfony/Bridge/Twig/Tests/Node/FormThemeTest.php

Lines 62 to 80 in8dcb1df


	$this->assertEquals(
	sprintf(
	'$this->env->getRuntime("Symfony\\\\Component\\\\Form\\\\FormRenderer")->setTheme(%s, [1 => "tpl1", 0 => "tpl2"], true);',
	$this->getVariableGetter('form')
	),
	trim($compiler->compile($node)->getSource())
	);

	$node =newFormThemeNode($form,$resources,0,null,true);

	$this->assertEquals(
	sprintf(
	'$this->env->getRuntime("Symfony\\\\Component\\\\Form\\\\FormRenderer")->setTheme(%s, [1 => "tpl1", 0 => "tpl2"], false);',
	$this->getVariableGetter('form')
	),
	trim($compiler->compile($node)->getSource())
	);

OskarStark changed the title~~[TwigBridge] Fix bootstrap 4 form error layout~~[TwigBridge] Fix Bootstrap 4 form error layout

Nov 3, 2023

asispts force-pushed thefix/bootstrap-4-form-layout branch 2 times, most recently fromc91e565 tod9d92e6Compare

November 12, 2023 19:16

Fix failed test in PHP 8.2 + high-deps

991255d

Copy link

ContributorAuthor

asispts commentedNov 15, 2023

This failed test doesn't use the recent change. The line inFormThemeTest.php:63 is a comment, and clearly, there's no[0 => "tpl1", 1 => "tpl2"] in the test code.

I can reproduce the failure in Symfony 4 and PHP 8.2 + high-deps. The result of(new Compiler($environment))->subcompile($node->getNode('resources'))->getSource()) is indeed["tpl1", "tpl2"]. While in the recent version of Symfony, the result is[1 => "tpl1", 0 => "tpl2"]

1) Symfony\Bridge\Twig\Tests\Node\FormThemeTest::testCompileFailed asserting that two strings are equal.--- Expected+++ Actual@@ @@-'$this->env->getRuntime("Symfony\\Component\\Form\\FormRenderer")->setTheme(($context["form"] ?? null), [0 => "tpl1", 1 => "tpl2"], true);'+'$this->env->getRuntime("Symfony\\Component\\Form\\FormRenderer")->setTheme(($context["form"] ?? null), ["tpl1", "tpl2"], true);'/home/runner/work/symfony/symfony/src/Symfony/Bridge/Twig/Tests/Node/FormThemeTest.php:63