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.

Sign up for GitHub

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

Jump to bottom

Deprecate help option in FieldDescription #6215

Merged
merged 1 commit into from
Aug 2, 2020
Merged

Deprecate help option in FieldDescription #6215

merged 1 commit into from
Aug 2, 2020

Conversation

franmomu
Copy link
Member

@franmomu franmomu commented Jul 21, 2020 •
edited
Loading

Subject

Ref: #6206 (comment)

I haven't seen any use case rather than showing help messages in form fields.

I am targeting this branch, because these changes are BC.

Closes #6207.

Changelog

### Deprecated
- Deprecated `BaseFieldDescription::setHelp()` and `BaseFieldDescription::getHelp()`.
- Deprecated passing `help` option to `BaseFieldDescription::setOptions()`.
- Deprecated `FormMapper::setHelps()` and `FormMapper:: addHelp()`.
- Deprecated passing `help` option to `FormMapper::add()` third argument containing HTML code without also passing `help_html` with `true` value.

I have to check if it works fine with nested fields.

@franmomu franmomu added the minor label Jul 21, 2020
franmomu
franmomu commented Jul 21, 2020
View reviewed changes
src/Form/FormMapper.php
if (isset($options['help'])) {
$fieldDescription->setHelp($options['help']);
unset($options['help']);
Copy link
Member Author

Choose a reason for hiding this comment

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

Symfony help option didn't work because the option was removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

description option

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Contributor

@kirya-dev kirya-dev Aug 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Truly says was:

Symfony help option didn't work because the description field option was removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm not exactly true, here at FormMapper it checked if the help form option was set, given:

->add('field', null, ['help' => 'message'])

the $options variable contains among other things ['help' => 'message'], then it executed:

$fieldDescription->setHelp($options['help']); so this help message is set as field description option and then:

unset($options['help']); which removes the form option, so it's never in the $options array that is passed to the form builder.

Copy link
Contributor

Choose a reason for hiding this comment

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

After you fix help dosnt unset from options

Copy link
Contributor

@kirya-dev kirya-dev Aug 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Dont miss difference between options and field description options

Copy link
Contributor

@kirya-dev kirya-dev Aug 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

You says:

Symfony help option didn't work because the option was removed.

And

Use Symfony Form "help" option instead.

This is mismatch statements

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok... I added that comment at the time I made the PR to explain the reason behind that change, so what I meant is... before making this change, Symfony Form "help" option didn't work as you expected (using ->add('field', null, ['help' => 'message'])) because that option was never passed as form option. After this change, you could use Symfony Form help option and it will work as expected.

franmomu
franmomu commented Jul 21, 2020
View reviewed changes
src/Resources/views/Form/form_admin_fields.html.twig
@@ -35,7 +35,7 @@ file that was distributed with this source code.

{% block form_help %}
{% apply spaceless %}
<span class="help-block sonata-ba-field-widget-help">{{ parent() }}</span>
<span class="help-block sonata-ba-field-widget-help sonata-ba-field-help">{{ parent() }}</span>
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this class because is the one used in the deprecated help.

phansys
phansys requested changes Jul 22, 2020
View reviewed changes
UPGRADE-3.x.md Outdated Show resolved Hide resolved
UPGRADE-3.x.md Outdated Show resolved Hide resolved
UPGRADE-3.x.md Outdated Show resolved Hide resolved
src/Admin/BaseFieldDescription.php Outdated Show resolved Hide resolved
src/Admin/BaseFieldDescription.php Outdated Show resolved Hide resolved
src/Form/FormMapper.php Outdated Show resolved Hide resolved
src/Form/FormMapper.php Outdated Show resolved Hide resolved
src/Form/FormMapper.php Outdated Show resolved Hide resolved
src/Form/FormMapper.php Outdated Show resolved Hide resolved
tests/Functional/Controller/CRUDControllerTest.php Outdated Show resolved Hide resolved
@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@franmomu franmomu force-pushed the deprecate_help_field_description branch 2 times, most recently from 12da595 to bea33c9 Compare July 24, 2020 20:57
@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@franmomu franmomu force-pushed the deprecate_help_field_description branch from bea33c9 to 392ce65 Compare July 28, 2020 16:25
@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@franmomu franmomu force-pushed the deprecate_help_field_description branch from 392ce65 to 970e196 Compare August 1, 2020 20:30
@franmomu franmomu marked this pull request as ready for review August 1, 2020 20:30
@jordisala1991 jordisala1991 merged commit 26a887d into sonata-project:3.x Aug 2, 2020
@jordisala1991
Copy link
Member

Thank you @franmomu

@franmomu franmomu deleted the deprecate_help_field_description branch August 2, 2020 10:08
@kirya-dev kirya-dev mentioned this pull request Aug 27, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kirya-dev kirya-dev kirya-dev left review comments

@phansys phansys phansys approved these changes

@jordisala1991 jordisala1991 jordisala1991 approved these changes

Assignees
No one assigned
Labels
minor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deprecate BaseFieldDescription::setHelp and BaseFieldDescription::getHelp
5 participants
@franmomu @SonataCI @jordisala1991 @phansys @kirya-dev