Skip to content

Update FormEnctypeNode.php to fix 12824 #12825

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

Closed
wants to merge 1 commit into from
Closed

Conversation

sstok
Copy link
Contributor

@sstok sstok commented Dec 3, 2014

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #12824
License MIT
Doc PR

I found that in #8731 the semi-colon was actually removed because of a problem with using the sandbox.
Which is properly why this was no longer noticed.

This is the simplest way to fix the syntax error.

@@ -22,7 +22,7 @@ class FormEnctypeNode extends SearchAndRenderBlockNode
public function compile(\Twig_Compiler $compiler)
{
parent::compile($compiler);

$compiler->write('trigger_error(\'The helper form_enctype(form) is deprecated since version 2.3 and will be removed in 3.0. Use form_start(form) instead.\', E_USER_DEPRECATED)');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will fix this ;)

@stof
Copy link
Member

stof commented Dec 3, 2014

I see an issue here anyway: This assumes that the function call is its own statement. Adding a semi-colon before will not fix the issue, it will just change it. The issue is that Twig does not ensure that function calls are their own statement (quite the opposite actually), so this way to trigger the deprecation notice is just broken.

add missing semi-colon
@sstok
Copy link
Contributor Author

sstok commented Dec 3, 2014

Any suggestion how this can be fixed properly?
The FormEnctypeNode can be changed to trigger the deprecation notice when it hits this Node, but that will only trigger the notice during compile time (and may potentially break the compiler).

@fabpot fabpot added the Form label Dec 7, 2014
@eXsio
Copy link

eXsio commented Dec 12, 2014

The problem persists and I don't see any clean way to trigger deprecation notice here. You could use some sort of a hack, like:

$compiler->raw('""; trigger_error(\'The helper form_enctype(form) is deprecated since version 2.3 and will be removed in 3.0. Use form_start(form) instead.\', E_USER_DEPRECATED); echo ');
parent::compile($compiler);

It would echo an empty string, trigger the warning and then go on with the compilation, but as I said it is ugly. I'm affraid, the only way to trigger the deprecation warning in a clean, nice matter would be to change

Symfony\Bridge\Twig\Extension\FormExtension

add something like

public function renderFormEnctype(FormView $form) {
        trigger_error('The helper form_enctype(form) is deprecated since version 2.3 and will be removed in 3.0. Use form_start(form) instead.', E_USER_DEPRECATED);
        return $this->renderer->searchAndRenderBlock($form, 'enctype');
}

and rewrite the 'form_enctype' function in getFunctions(), to use the above function instead of FormEnctypeNode (which then could be deleted). The alternative is to remove the deprecation warning. Either way something has to be done, beacause the current code breaks form_enctype completely.

@dbu
Copy link
Contributor

dbu commented Dec 26, 2014

if there is no proper solution, can we just revert the change? this is completely breaking things, as in Parse error: syntax error, unexpected 'trigger_error' (T_STRING), expecting ',' or ';' in ...

that way, it is certainly and utterly pointless breaking BC...

@dbu
Copy link
Contributor

dbu commented Dec 26, 2014

did #13125 to re-add the call to raw. lets either merge that, remove the deprecation or find a better fix but not leave code in here that triggers a fatal error. heck, even die('Do not use form_enctype anymore.'); would be more user friendly than what we currently have.

@fabpot
Copy link
Member

fabpot commented Dec 26, 2014

As there is no way to get it right, I've reverted #12673

@fabpot fabpot closed this Dec 26, 2014
@dbu
Copy link
Contributor

dbu commented Dec 26, 2014

alright, thanks fabien!

@sstok sstok deleted the patch-6 branch April 9, 2017 16:26
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.

5 participants