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

[WIP] Adding a resource form type #1772

Merged
merged 1 commit into from
Aug 21, 2014

Conversation

arnolanglade
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Fixed tickets -
License MIT
Doc PR No yet

I already submit a PR like that but it was a little bit different. I use resource bundle for custom development and I create this form all the time, it is really annoying! If you are agree with this I can update the Sylius's form in a other PR.

$options['validation_groups'] = $this->validationGroups;
}

$resolver->setDefaults($options);
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO good enough would be:

$resolver->setDefaults(array(
    'data_class'        => $this->dataClass,
    'validation_groups' => $this->validationGroups,
));

Cause anyway you have validation done on resolver (if not we should add it).

@stloyd
Copy link
Contributor

stloyd commented Aug 8, 2014

If we go this way, that class should be in ResourceBundle.

@arnolanglade
Copy link
Contributor Author

@stloyd For sure! I don't know what I did... SettingBundle....

@arnolanglade
Copy link
Contributor Author

@stloyd, Done. @pjedrzejewski what do you think ?

protected $dataClass = null;

/**
* @var array
Copy link
Contributor

Choose a reason for hiding this comment

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

string[]

@stloyd
Copy link
Contributor

stloyd commented Aug 9, 2014

Looks good, I guess you should adjust forms in repository to reduce the duplication ;)

@arnolanglade
Copy link
Contributor Author

@stloyd Every forms should be updated. @pjedrzejewski this one? or I open another one?

@pjedrzejewski
Copy link
Member

Sounds good to me. I am fine with adjusting them in this PR. I will make sure to merge it quick and avoid conflicts.

@arnolanglade
Copy link
Contributor Author

@pjedrzejewski I will updated ASAP (I am in holidays for a week)

@arnolanglade
Copy link
Contributor Author

@pjedrzejewski, @stloyd, sometime form only need data class, sometime only they need validation groups or they need both... Setter could be better but the service definitions should be rewritten... Both solutions are huge... I just update form which used data class or the both? Any opinion on this ?

@arnolanglade arnolanglade changed the title Adding a resource form type [WIP] Adding a resource form type Aug 10, 2014
@arnolanglade
Copy link
Contributor Author

Some form type constructors change, I need to manage it.

protected $actionRegistry;

public function __construct($dataClass, array $validationGroups, ServiceRegistryInterface $actionRegistry)
{
$this->dataClass = $dataClass;
$this->validationGroups = $validationGroups;
parent::__construct($dataClass, $validationGroups, $actionRegistry);
Copy link
Contributor

Choose a reason for hiding this comment

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

$actionRegistry must be removed here.

@arnolanglade
Copy link
Contributor Author

@stloyd Fixed :)

@stloyd
Copy link
Contributor

stloyd commented Aug 20, 2014

@pjedrzejewski 👍

@arnolanglade
Copy link
Contributor Author

@pjedrzejewski what do you about it ?

pjedrzejewski pushed a commit that referenced this pull request Aug 21, 2014
[WIP] Adding a resource form type
@pjedrzejewski pjedrzejewski merged commit b2b4776 into Sylius:master Aug 21, 2014
@pjedrzejewski
Copy link
Member

@Arn0d I love the diff stats, it is a good refactoring. ;) Thank you very much! 👍

@arnolanglade arnolanglade deleted the resource/base-form branch December 16, 2014 12:12
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.

None yet

3 participants