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

Rename FormField NS to Form\Control (and FormLayout to Form\Layout) #1311

Merged
merged 37 commits into from
Jun 26, 2020

Conversation

georgehristov
Copy link
Collaborator

@georgehristov georgehristov commented Jun 23, 2020

fixes #794

Introduce Form namespace

  • Elegant and clear syntax when importing the atk4\ui\Form namespace only
use atk4\ui\Form;

new Form\Control\Checkbox();
new Form\Layout\Columns();
  • removes duplicate class names like Generic

  • renames classes according to PSR

  • docs updated with explicit variable naming and new namespace importing for clarity, e.g.
    $group->addField('gender', [Form\Control\Dropdown::class, 'values' => ['Female', 'Male']]);
    $f -> $form, $gr -> $group, etc

BC break:

  • instanceof operator returns false if tested against the old class names

Autoload of old classes will trigger DEPRECATED warning

Migration:

Simple regex: (?<!\w)FormField(?!\w)

Search/replace in files

  1. class FormField\Generic to Form\Control
  2. class FormLayout\Generic to Form\Layout
  3. namespace atk4\ui\FormField to atk4\ui\Form\Control
  4. namespace atk4\ui\FormLayout to Form\Layout
  5. class FormField\CheckBox to Form\Control\Checkbox (change of class name case only)
  6. class FormField\DropDown to Form\Control\Dropdown (change of class name case only)
  7. class FormField\DropDownCascade to Form\Control\DropdownCascade (change of class name case only)
  8. class FormField\MultiLine to Form\Control\Multiline (change of class name case only)
  9. class FormField\UploadImg to Form\Control\UploadImage
  10. class FormField\TextArea to Form\Control\Textarea (change of class name case only)

@PhilippGrashoff
Copy link
Collaborator

I like the idea about the Form namespace. However, I know at least of 3 people (me included) that confused Model Fields and Form Fields when starting with ATK. My idea to avoid this was to refer to Form Fields as FormField always (Docs, Methods, whatever). So Form would have getFormFields(), getFormField() methods etc.

I doubt having Form/Field namespace alone will do the job to avoid this confusion in future. How about Form/FormField?

Also see #794

@georgehristov
Copy link
Collaborator Author

georgehristov commented Jun 23, 2020

@PhilippGrashoff the idea is to force always use the Form namespace which will also solve the problem for newcomers
So you declare use atk4\ui\Form and then use Form\Field or From\Field\Checkbox for instance.
I have changed all ui demos accordingly so demos code is self explanatory in this regard and avoids confusion.
Same will be refactored for atk4/data and Model namespace introduced so you will use Model\Field or Model\Field\Boolean for instance to refer to model fields.
I think this solves the problem in a very elegant way using power of php namespaces.

@georgehristov georgehristov linked an issue Jun 23, 2020 that may be closed by this pull request
@georgehristov georgehristov force-pushed the refactor/form-namespace branch 2 times, most recently from b04ed01 to 831f34a Compare June 24, 2020 06:59
@mvorisek
Copy link
Member

@georgehristov FormField rename - this is valid only if you can guarantee than FormField can not be used on its own without a Form - is this 100% true?

Based on my search it is currently used at least in Grid for searchbox /wo Form. Please comment on this case.

Based on my research the /wo

is valid by html spec - https://stackoverflow.com/questions/3294572/is-input-well-formed-without-a-form . With the same reasoning, the FormLayout can (does it work now in Atk now?) be used without Form probably too.

This is also related with my PR #1275 to support nested forms. We may simply always require a form to support all features. Only with this approach we can guarantee, that the input in for. ex. Grid was not mistakely wrapped inside another form (thus it is not serialized etc).

How does other popular frameworks, even not PHP ones, solves this?

@georgehristov
Copy link
Collaborator Author

We are mixing several issues here:

  • first impression / learning curve
    This is purely DX / perception issue. By introducing a clear structure and importing only Form namespace in demos / docs / code we keep it clear to what the code is referring to From\Field\Checkbox instead of Checkbox only.
    Model fields will be Model\Field\Boolean for instance.
    Devs can then implement imports as they like in their code but many times this is influenced by the base code of the toolkit/framework.
    One needs to understand what model and form are from the docs and then which field is used in the demos.
    My other idea of calling Form\Field something different (like Form\Input) was rejected. This would put even more distinction and clarity IMHO.
    So apart from that I believe this is as clear as it gets for avoiding confusion.

  • functionality
    ATK now supports using form field without attaching it to a form. This is a matter of a different discussion.

@mvorisek
Copy link
Member

ATK now supports using form field without attaching it to a form. This is a matter of a different discussion.

it is not, because this can very easily imply that decision to move it under Form NS si wrong - it can be used without Form, it is just one of a View building block addable to anywhere, element, which can be used in Form but you do not have too.

But I think this is a step in the right direction and there are no additional downsides (like with your other refactoring PRs). I will check now.

After mergin this, we should:

src/Form/Field/Input.php Outdated Show resolved Hide resolved
src/Form/Field/Upload.php Outdated Show resolved Hide resolved
@mvorisek mvorisek changed the title Rename FormField NS to Form\Field (and FormLayout to Form\Layout) Rename FormField NS to Form\Control (and FormLayout to Form\Layout) Jun 25, 2020
@mvorisek mvorisek added the RTM label Jun 26, 2020
src/Form.php Outdated Show resolved Hide resolved
@mvorisek
Copy link
Member

mvorisek commented Jun 26, 2020

two cosmetic changes to fix, then we can merge it this afternoon because it is almost BC

@mvorisek mvorisek merged commit 26bc53f into develop Jun 26, 2020
@mvorisek mvorisek deleted the refactor/form-namespace branch June 26, 2020 10:31
@PhilippGrashoff
Copy link
Collaborator

Ah, Control is nice :)

@mvorisek
Copy link
Member

@PhilippGrashoff thanks for feedback, I mean for both! :D

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

Successfully merging this pull request may close these issues.

Docs: Refer to Form Fields as 'FormField' always?
4 participants