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

Added support for form fields in containers #380

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lulco
Copy link
Contributor

@lulco lulco commented Jun 5, 2023

Collecting container fields to container based on its name and variable name.

This will work:

protected function createComponentContainerForm(): Form
{
    $form = new Form();
    $container = $form->addContainer('container');
    $container->addText('text1', 'Text 1');
    $container->addText('text2', 'Text 2');
    return $form;
}

This will not work:

protected function createComponentContainerForm(): Form
{
    $form = new Form();
    $foo = $form->addContainer('container');
    $foo->addText('text1', 'Text 1');
    $foo->addText('text2', 'Text 2');
    return $form;
}

because 'foo' !== 'container'

@lulco
Copy link
Contributor Author

lulco commented Jun 5, 2023

self-note
TODO: add node visitor to change $form["part1-text1"] to $form["part1"]["text1"] and add tests

@lulco
Copy link
Contributor Author

lulco commented Jun 5, 2023

follow up:
collect assigns:

$container = $form->addContainer('container_name');
and use these container => container_name in FormControlFinder

@MartinMystikJonas
Copy link
Collaborator

I will check in more detail later but I think this can cause some problems.

  1. Would this raise fale positives when variable name is not same as container name?
  2. What if multiple containers use same variable name?
  3. What about containers in containers?

@lulco
Copy link
Contributor Author

lulco commented Jun 6, 2023

I will check in more detail later but I think this can cause some problems.

  1. Would this raise fale positives when variable name is not same as container name?

yes, I still think this is much better than actual state where everything is considered to be forms's field, we can improve it in some next PRs - like mentioned above I'm planning to collect container names with variable names and use them to cover these use cases

  1. What if multiple containers use same variable name?

In actual implementation, it will not work, because variable name will not be the same as container name. It is limitation which is documented. In next implementation with collected couples variable with container name it will not work too because there will be some conflict. Code of application have to be changed or error ignored.

  1. What about containers in containers?

not yet implemented, but it should be the same - just some recursion

I want to keep PRs small with only one main change.

@MartinMystikJonas
Copy link
Collaborator

My concern is if it would not cause too many false positives.

@lulco
Copy link
Contributor Author

lulco commented Jun 6, 2023

My concern is if it would not cause too many false positives.

we need to test in real world apps

@MartinMystikJonas
Copy link
Collaborator

I think in our apps on most places container name and variable name would not match and we use containers in containers in many places because we have some custom multi-part inputs (containers with few inputs like comparsion+value, hour+minute, etc.)

Maybe to avoid fale positives we should keep falldack to current behaviour (if control is not found in properly named container then it is searched everywhere)

@lulco
Copy link
Contributor Author

lulco commented Jun 6, 2023

I think in our apps on most places container name and variable name would not match and we use containers in containers in many places because we have some custom multi-part inputs (containers with few inputs like comparsion+value, hour+minute, etc.)

Maybe to avoid fale positives we should keep falldack to current behaviour (if control is not found in properly named container then it is searched everywhere)

But original behavior causes many false negatives, that's why I'm reworking it now. Also with multi part inputs and containers in containers I would suggest you to use renderer instead of custom latte rendering

@MartinMystikJonas
Copy link
Collaborator

Maybe make this a optional feature then? I would like to avoid many false positives in defautl behaviour but it is true that ignoring false negatives is also not ideal.

@lulco
Copy link
Contributor Author

lulco commented Jun 6, 2023

I have to check, but I think these two solutions are not compatible and it doubles the code we have to support

@MartinMystikJonas
Copy link
Collaborator

Maybe implement it in a way that would optionaly allow fallback. Something like creating "virtual" container '*' and use ($form['container']['control'] ?? $form ['*']['control']])

spaze added a commit to spaze/michalspacek.cz that referenced this pull request Jul 15, 2023
[9 errors remaining](https://github.com/spaze/michalspacek.cz/actions/runs/5562124085/jobs/10160242058)
and they all seem to be related one way or another to form containers,
so maybe the following will help?
- efabrica-team/phpstan-latte#380

Ref #141
spaze added a commit to spaze/michalspacek.cz that referenced this pull request Jul 17, 2023
…pported

I hope these would be solved one day when efabrica-team/phpstan-latte#380 is closed but currently, even when testing that branch, aren't.
spaze added a commit to spaze/michalspacek.cz that referenced this pull request Jul 17, 2023
Ignoring just errors related to form containers which are not yet (fully) supported.

I hope these would be solved one day when efabrica-team/phpstan-latte#380 is closed but currently, even when testing that branch, aren't.

Ref #141
@lulco lulco marked this pull request as draft July 24, 2023 21:11
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

Successfully merging this pull request may close these issues.

None yet

2 participants