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
3.0 form idprefix #2980
3.0 form idprefix #2980
Conversation
@@ -43,6 +44,8 @@ | |||
*/ | |||
class FormHelper extends Helper { | |||
|
|||
use IdGeneratorTrait; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need a newline here
This looks good to me 👍 |
|
||
$this->Form->create(false, ['idPrefix' => 'prefix']); | ||
|
||
$this->Form->expects($this->once())->method('datetime') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The datetime widget doesn't use the idPrefix option. Shoud it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Datetime widget doesn't use the IdGeneratorTrait itself.
I believe there was a discussion earlier to just skip id generation for datetime selects for now since we wouldn't know for which select the label's for
attribute should be set if selects for all datetime parts are not generated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, that is the one remaining incomplete test for formhelper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anything else needs to be done before this can be merged? Datetime can be dealt with later on.
Is it ok to keep IdGeneratorTrait
under Widget now that it's used for helper too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to move the trait to View/Helper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
3.0 form idprefix Fixes #2952
Refs #2952