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

Conflict with [FrameworkBundle] Added Crontoller::isCsrfTokenValid [SF 2.6.0] #789

Open
enkaho opened this issue Sep 9, 2014 · 25 comments

Comments

@enkaho
Copy link
Contributor

enkaho commented Sep 9, 2014

AdmingeneratorGeneratorBundle / Resources / templates / CommonAdmin / csrf_protection.php.twig

cf : symfony/symfony@479c833

@ioleo
Copy link
Member

ioleo commented Sep 9, 2014

@enkaho thanks for the notice

@sescandell
Copy link
Member

Thanks...

Unfortunatelly... the only way I see for now to handle that change is to introduce a BC Break.
Does anyone has a (simple) idea to avoid this BC?

I thought about testing the Symfony version and if it is less then 2.5: we don't change anything, otherwise the function will be different, but we still have a BC break for 2.6...

If we strictly want to respect semver, we need to create a new Major version... What's your opinion?

@sescandell
Copy link
Member

@sescandell
Copy link
Member

ping @loostro

Matter here is, depending on your Symfony version, we don't generate the same method name... a kind of BC Break (if you overrided the isCsrfTokenValid()).

But I was wondering something... based on the situation where someone upgrade its Symfony to 2.6, the bundle will not be anymore compatible with it. So, except by a BC Break it will not be able to use this bundle anymore. But given that the developper make an upgrade of Symfony2.6, we can assume he will have to change its customization... so we can easily imagining changing our function and so provide compatibilities with Symfony 2.6+

Otherwise, we'll need to update composer.json and constraint the max version of Symfony to <2.6

I don't have any clear idea about what we can do here...
I think the better would be to "introduce" this BC Break (with a very clear explanation). I think impact is limited (I don't think many user override the isCsrfTokenValid() function).
Or do we need to create a 2.0.0 version... just for that... that will be complicated to maintain many versions...

@ioleo
Copy link
Member

ioleo commented Oct 13, 2014

@sescandell We can do two things:

  • create a doc with modified csrf_token.php.twig code to copy&paste to app/Resources/Admingenerator/GeneratorBundle/templates/CommonAdmin for sf2.6+ users
  • explain in this doc entry, why we could not merge this into master (and sf2.6+ users have to copy the code)

And when next LTS is released (2.7) then merge this patch into repository.

@sescandell
Copy link
Member

Sounds like a good idea...

Let's do it

@robertfausk
Copy link
Contributor

@sescandell @loostro What need csrf_token.php.twig to look like? Is there a base template to inherit from? I couldn't find one.
I'm in need of upgrade to sf2.6 but the AdminGeneratorBundle causes trouble.

@ioleo
Copy link
Member

ioleo commented Feb 12, 2015

@robertfausk see this changelog

@robertfausk
Copy link
Contributor

Yeah, I already did. But I have no luck with overriding.

I placed this

{% block csrf_protection_use %}
use Symfony\Component\Security\Core\Exception\InvalidCsrfTokenException;
{% endblock %}

{% block csrf_check_token %}

    /**
     * Check crsf token provided for action
     *
     * @throw InvalidCsrfTokenException if token is invalid
     */
    protected function isCsrfTokenValid($intention)
    {
        $token = $this->getRequest()->request->get('_csrf_token');
        if (!$this->get('form.csrf_provider')->isCsrfTokenValid($intention, $token)) {
            throw new InvalidCsrfTokenException();
        }
    }

{% endblock %}

{% block csrf_action_check_token %}
// Check CSRF token for action
$intention = $this->getRequest()->getRequestUri();
$this->isCsrfTokenValid($intention);
{% endblock %}

{% block csrf_action_check_batch_token %}
// Check CSRF token for batch action
$intention = '{{ namespace_prefix }}_{{ bundle_name ~ (builder.BaseGeneratorName ? '_' ~ builder.BaseGeneratorName :'')}}_batch';
$this->isCsrfTokenValid($intention);
{% endblock %}

in app/Resources/AdmingeneratorGeneratorBundle/templates/CommonAdmin/csrf_protection.php.twig but it doesn't override the template.

I had also no luck with app/Resources/Admingenerator/GeneratorBundle/templates/CommonAdmin/csrf_protection.php.twig like you suggested in #789 (comment)

@sescandell
Copy link
Member

Hi @robertfausk

Actually to "override" a template you need to add your "templates" directory as a config parameter. Take a look to the param templates_dirs https://github.com/symfony2admingenerator/AdmingeneratorGeneratorBundle/blob/master/DependencyInjection/Configuration.php#L72

@robertfausk
Copy link
Contributor

@sescandell Thank you very much. I got it now. Works like a charm ;)

Additionally I had to create one emtpy dir named Doctrine (or DoctrineODM or Propel) to get it working.
Also see doc: https://github.com/symfony2admingenerator/AdmingeneratorGeneratorBundle/blob/master/Resources/doc/cookbook/extending-generator-templates.md

@sescandell
Copy link
Member

You're right... I forgot to mention this.

@bobvandevijver
Copy link
Member

@robertfausk Exactly the reason I've written that documentation: A long time ago I had the same problem ;)

@fjbatresv
Copy link

Hello I am trying to edit the templates, i have a SF 2.6, Here is my directory

app
     Resources
          Admingenerator
               GeneratorBundle
                    templates
                         CommonAdmin
                              *templates*
                         Propel

And on my config:

templates_dirs: ["%kernel.root_dir%/../app/Resources/AdmingeneratorGeneratorBundle/templates" ]

Can someone explainme why this does not work properly.

@sescandell
Copy link
Member

@fjbatresv if you are working on Windows, add a file in your Propel directory (even if it is empty), and it should work.

@fjbatresv
Copy link

@sescandell I am working on a linux distibution and this doesnt work.

@sescandell
Copy link
Member

OK...

I think this is because of your path. You should use this:
templates_dirs: ["%kernel.root_dir%/../app/Resources/Admingenerator/GeneratorBundle/templates" ]

@sebastianlp
Copy link

I think with Symfony 2.7.* this doesn't work any more :(

This is my folder structure:

app
 Resources
  Admingenerator
   GeneratorBundle
    templates
     CommonAdmin
      csrf_protection.php.twig (with proper content)
     Doctrine
      .gitkeep

And my config.yml

admingenerator_generator:
    [...]
    templates_dirs: [ "%kernel.root_dir%/../app/Resources/templates" ]

Am I doing something wrong here?

@bobvandevijver
Copy link
Member

@sebastianlp Check if you made every step as described here.

Futhermore, the fix is, if I'm correct, now being developed in the newer version of the Bundle (see here. So, it might actually be time for you to update to the new bundle, also to benefit of new functions.

@sescandell
Copy link
Member

@sebastianlp

Your path should be "%kernel.root_dir%/../app/Resources/Admingenerator/GeneratorBundle/templates"

@sebastianlp
Copy link

@bobvandevijver , @sescandell
Yeah, I forgot to update the answer. I'm using:

admingenerator_generator:
    templates_dirs: [ "%kernel.root_dir%/../app/Resources/Admingenerator/GeneratorBundle/templates" ]

and this is my folder structure:
screenshot from 2015-06-22 22 10 00

Thank you guys

@bobvandevijver
Copy link
Member

In te docs the directory is mentioned as AdmingeneratorGeneratorBundle, so without the extra subdir per Symfony convention. I also have that and it works, so you could try that.

@sescandell
Copy link
Member

OK... I'll make a test on the demo project moving on sf2.7 and let you know.

@sebastianlp
Copy link

@bobvandevijver Thank you. I tried both paths, that one in the image and what you mention in the last comment, both with error.

@sescandell Thank you, if it's more convenient to you, I can upload my project

@sescandell
Copy link
Member

Working on it.

I'll probably be able to provide you a response tomorrow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants