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

[ResourceBundle] Automatic form registration #1927

Merged

Conversation

Strontium-90
Copy link
Contributor

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

Register resource form types as services.
Its possible register many form for resourse, i.e:

classes:
    controller: MyControllerClass
    form:
        default:             MyBundle\Form\Type\AnyResourceType
        short:                MyBundle\Form\Type\AnyResourceShortType
    choice_form:        MyBundle\Form\Type\AnyResourceChoiceType

@Strontium-90 Strontium-90 force-pushed the automatic-form-registration branch 2 times, most recently from 9583b83 to 2108b07 Compare October 12, 2014 08:23
@arnolanglade arnolanglade added Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). ResourceBundle labels Oct 12, 2014
@@ -134,8 +176,7 @@ protected function getClassMetadataDefinition($models)
->setFactoryService($this->getManagerServiceKey())
->setFactoryMethod('getClassMetadata')
->setArguments(array($models))
->setPublic(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be reverted

@arnolanglade
Copy link
Contributor

@Sylius/core-team What do you think about it ?

@Strontium-90
Copy link
Contributor Author

Reworked and added specs.


$container
->getDefinition('sylius.form.type.province_choice')
->setArguments([
Copy link
Contributor

Choose a reason for hiding this comment

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

We supports PHP 5.3 :)

@arnolanglade
Copy link
Contributor

@pjedrzejewski The functionality is very interesting, I would want it into the resource bundle!! @Strontium-90 Why do you merge #2012 into this PR? Can you revert it and squash your commit I will review it again.

@pjedrzejewski
Copy link
Member

Looks really interesting, but please do what @Arn0d suggested, #2012 is a bit different thing, I'd like to evaluate them separately. Thanks and nice work! 👍

public function __construct($className, $driver, $name)
{
$this->className = $className;
$this->parent = $driver;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is useless as you assign this variable in switch later...

@Strontium-90 Strontium-90 force-pushed the automatic-form-registration branch 3 times, most recently from 6ab7b58 to c14bb9d Compare November 4, 2014 11:09
@Strontium-90 Strontium-90 force-pushed the automatic-form-registration branch 2 times, most recently from 45b1df4 to 48ed8e2 Compare December 19, 2014 05:30
@Strontium-90
Copy link
Contributor Author

You listed 1. and 1. but I pick the 2nd. 1. :)

@pjedrzejewski, @Arn0d, I reworked to this config, rebased... Any review and lets finish here?

$suffix = ($name === self::DEFAULT_KEY ? '' : sprintf('_%s', $name));
$alias = sprintf('%s_%s%s', $this->applicationName, $model, $suffix);
$definition = new Definition($class);
if ($name == 'choice') {
Copy link
Member

Choose a reason for hiding this comment

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

'choice' === $name.

@pjedrzejewski
Copy link
Member

Awesome work Aleksey! Please tweak these few things I have mentioned + rebase and I will merge it. :) Thank you! 👍

@arnolanglade
Copy link
Contributor

I had a quick look, it seems good!

@Strontium-90
Copy link
Contributor Author

done

@pjedrzejewski
Copy link
Member

Seems like there are still conflicts. :)

@Strontium-90 Strontium-90 force-pushed the automatic-form-registration branch 2 times, most recently from 97b2021 to 7e8af31 Compare December 19, 2014 10:44
@Strontium-90
Copy link
Contributor Author

Rebased again.

@pjedrzejewski
Copy link
Member

Could you have a look why scenarios are failing? Master is green.

@Strontium-90
Copy link
Contributor Author

Master is green.

Good news.

Almost green, but some suites exited with 137...

pjedrzejewski pushed a commit that referenced this pull request Jan 3, 2015
[ResourceBundle] Automatic form registration
@pjedrzejewski pjedrzejewski merged commit 4446a8b into Sylius:master Jan 3, 2015
@pjedrzejewski
Copy link
Member

Thank you so much Aleksey! This is a really cool thing, I will remove other forms. :) 👍

@Strontium-90
Copy link
Contributor Author

I will remove other forms. :)

I think this will more easy, if we finish this PR #1921.

@adamelso
Copy link
Contributor

This change is nice, I like it. However there's something slightly counter-intuitive in having to manipulate the service definitions just to pass in dependencies:

$container
    ->getDefinition('my.form.type.blog')
    ->setArguments(array(
        new Reference('my.repository.blog')
    ))
;

Perhaps in another PR, this can be exposed to the config?

@isometriks
Copy link
Contributor

I agree with @adamelso, I think it would be easier to have a check to see if the form's definition already exists and if so, it doesn't create it. That way any form that isn't standard you can create in your service definitions and any simple ones will just get created by themselves. It seems silly to mess with the definitions programmatically only. Kind of adds a big learning curve for newer users and is harder to read and trace out errors. Should be simple to change it to this:

    protected function registerFormTypes(array $config, ContainerBuilder $container)
    {
        foreach ($config['classes'] as $model => $serviceClasses) {
            if (!isset($serviceClasses['form']) || !is_array($serviceClasses['form'])) {
                continue;
            }
            foreach ($serviceClasses['form'] as $name => $class) {
                $suffix = ($name === self::DEFAULT_KEY ? '' : sprintf('_%s', $name));
                $alias = sprintf('%s_%s%s', $this->applicationName, $model, $suffix);
                $definitionId = sprintf('%s.form.type.%s%s', $this->applicationName, $model, $suffix);

                // HERE
                if ($container->hasDefinition($definitionId)) {
                    continue;
                }

                $definition = new Definition($class);
                if ('choice' === $name) {
                    $definition->setArguments(array(
                        $serviceClasses['model'],
                        $config['driver'],
                        $alias
                    ));
                } else {
                    $definition->setArguments(array(
                        $serviceClasses['model'],
                        new Parameter(sprintf('%s.validation_group.%s%s', $this->applicationName, $model, $suffix))
                    ));
                }
                $definition->addTag('form.type',array('alias' => $alias));
                $container->setDefinition(
                    $definitionId,
                    $definition
                );
            }
        }
    }

@liverbool
Copy link
Contributor

@isometriks I've use some your code snippet in my PR #2483 .

@Strontium-90 Strontium-90 deleted the automatic-form-registration branch June 4, 2015 13:06
pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019
…tration

[ResourceBundle] Automatic form registration
pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019
…tration

[ResourceBundle] Automatic form registration
pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019
…tration

[ResourceBundle] Automatic form registration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants