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

Integrate Froala with Sonata #17

Closed
Khaldoun488 opened this issue May 23, 2016 · 19 comments
Closed

Integrate Froala with Sonata #17

Khaldoun488 opened this issue May 23, 2016 · 19 comments

Comments

@Khaldoun488
Copy link

Khaldoun488 commented May 23, 2016

Hey

I am used Froala Editor with Sonata Admin

$formMapper->add('content', 'froala');

And I have this exception :

Impossible to access an attribute ("options") on a null variable in SonataAdminBundle:Form:form_admin_fields.html.twig at line 326

@sguionni
Copy link
Contributor

Hey,
the error displayed seems to occur in a Sonata view, not in Froala.

@es11400
Copy link

es11400 commented Sep 8, 2016

I do not understand your answer, you do not help him.

¿Froala supports Admin Sonata?

@sguionni
Copy link
Contributor

sguionni commented Sep 9, 2016

This issue describes an error in sonata form, i cant do anything without more informations or code.

@maryamezzaty24
Copy link

I have this problem too
can you give me a solution?

@gotzmann
Copy link

Are there any solution for this issue?

@jordisala1991
Copy link

https://github.com/froala/KMSFroalaEditorBundle/blob/master/Form/Type/FroalaEditorType.php#L98

Are you removing every custom paramter for the view that does not come from Froala? That could cause issues with how Sonata render forms, probably

@sguionni sguionni reopened this Mar 29, 2018
@bhtomming
Copy link

I have the same error "Impossible to access an attribute ("options") on a null variable."
in sonataAdminController method like this:

use KMS\FroalaEditorBundle\Form\Type\FroalaEditorType;
public function configureFormFields(FormMapper $form){
$form->add('description',FroalaEditorType::class)
}
I dont know what happend!

@l396635210
Copy link

l396635210 commented Mar 20, 2020

I have the same error in SonataAdmin, and I
remove this file 'vendor/kms/froala-editor-bundle/Form/Type/FroalaEditorType.php'
this function buildForm (line 69 - 78) body content

			{
				$p_builder->setAttribute( $key, $option );
			}
```,
it can work, but I don't know it Destructive force

@mpoiriert
Copy link
Contributor

The problem is that the body content override the attribute with all options parameters.

The FroalaEditorType didn't have been developed with the symfony form extension in mind so it didn't escape it's configuration (by adding a prefix on the options/attributes name) and at the same time might override attribute set by other extension since they are treating all options bass to the build form, only the one that they are "owner" off should be process.

@mpoiriert
Copy link
Contributor

mpoiriert commented Jul 16, 2020

@sguionni This is clearly not a problem with sonata but rather how this bundle have been implemented, I think the invalid label should be removed and bug should be addressed since it could impact any Symfony form extension

@jmsche
Copy link
Contributor

jmsche commented Jul 16, 2020

Hi @mpoiriert, can you tell more about how this breaks Sonata?

Could you make a tiny reproducer for this? Thanks!

@mpoiriert
Copy link
Contributor

mpoiriert commented Jul 16, 2020

You have the step to reproduce here: sonata-project/SonataAdminBundle#4773

In the symfony form system you can have extension (which Sonata Use) for example to add options.

When you receive the options (like in the buildForm and buildView method) you get ALL the options of ALL the form extension and your own.

Since you do this in the buildForm

foreach( $p_options as $key => $option )
                $p_builder->setAttribute( $key, $option );
}

And sonata has a 'sonata_admin' option and the extension is execute before you code you override the value that have been set by sonata by (if I put the variable clear) this: $p_builder->setAttribute('sonata_admin', $option);

Since most (will say all the time) the option by default for sanata_admin is 'null' you set the attribute sonata_admin to null.

The buildForm does this to all attribute setting of all extension.

Some extension might not use the option name as a attribute so they do no interfer.

What you should have done is only loop trough options you have define in the FroalaEditorType.

Another problem is that you define a lot of options with really common name (profile, autofocus, iframe, etc.) and you can have conflict with your options and other extension.

What should have been done is prefix all of them with "froala_" so there is no conflict possible.

This will obliviously not be backward compatible.

Also from a development point of view some IDE (like PHPStorm) offer auto-completion on option when defining for. For this to work you need to set them clearly:

public function configureOptions( OptionsResolver $p_resolver )
{
    $p_resolver->setDefined([
      'froala_profile',
     'froala_iframe',
    //etc.
    ]);
}

By suing a loop and method call and other thing, you might have less maintenance to do if you are reusing them but it reduce the usability for the developers.

@jmsche
Copy link
Contributor

jmsche commented Jul 16, 2020

I think the right fix is, for now, to loop only on options defined in Froala bundle and use the setAttribute() only on these instead of all options. Then your issue in Sonata would be fixed as well. WDYT?

Another problem is that you define a lot of options with really common name (profile, autofocus, iframe, etc.) and you can have conflict with your options and other extension.
What should have been done is prefix all of them with "froala_" so there is no conflict possible.

I'm quite against the prefix as these options are meant for the FroalaEditorType in particular.

I have checked a few (popular) bundles, none is using prefixes for their form options.

About defining options in the FroalaEditorType: that would be quite a lot to handle on two sides (FormType & DI) but maybe this should be done.

I'll try to create a reproducer using SonataAdmin & Froala bundles, and if I manage to do that I'll check that the fix works.

I'd happily talk about this more on Symfony Slack if that's an option for you :)

@mpoiriert
Copy link
Contributor

I have already made a fork and fix what I needed to work.

I understand your point about the prefix but this is not a issue when you have really specific option. But imagine having another extension that have a "profile" to define the profile of their configuration they will be in conflict, includeJS/includeCSS is another good example, what if a extension have js/css too and need to include them but not in yours ? I am pretty sure I can find many other option that can potentially conflict by their name.

You don't need to namespace them when you are sure they are unique or you want to override a behaviour that would refer the same behaviour (a placeholder option would refer to the placeholder option of a form input so this would make sens to not prefix it). I never saw a form extension with 50 options, that is not common. Probably you should use a array configuration in that case to group your "editor options" together. Yours have a high chance of conflict that's why I suggest namespace prefix.

@gansky-alexander
Copy link

Hello, guys ! Do you have any updates regarding Froala and Sonata integration ?

@mpoiriert
Copy link
Contributor

mpoiriert commented Sep 19, 2020 via email

@gansky-alexander
Copy link

Thank you, @mpoiriert ! I will check

@gansky-alexander
Copy link

Do you mean this one, right?
https://github.com/mpoiriert/KMSFroalaEditorBundle

@jmsche
Copy link
Contributor

jmsche commented Dec 7, 2020

Should be fixed with 4.0.0 release

@sguionni sguionni closed this as completed Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests