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

Set default idPrefix for Form Helper #6134

Closed
DSchalla opened this issue Mar 22, 2015 · 10 comments · Fixed by #6153
Closed

Set default idPrefix for Form Helper #6134

DSchalla opened this issue Mar 22, 2015 · 10 comments · Fixed by #6153
Milestone

Comments

@DSchalla
Copy link

Since the default idPrefix for the FormHelper is "null", this can lead to id collisions on a website.

As example:
I got an DB with the columns id, username, title, content, so the FormHelper creates all inputs by default with:
<label for="content">...<//label><textarea name="content" id="content"></textarea>

Since content is a common id for the content wrapper etc and those collisions might come up also with other ids, I would say it makes sense to set the default idPrefix for the FormHelper to "cakeForm" to reach relative unique ids like "cakeForm-title", "cakeForm-content" etc to avoid collisions at all.

The ID Prefix above was just an example, you get the idea.

@dereuromark dereuromark added this to the 3.0.0 milestone Mar 22, 2015
@dereuromark
Copy link
Member

I agree that we should be closer to the uniqueness 2.x provided to avoid unnecessary collisions, which seem to be quite likely with just the flat data array and thus the field names directly.

It might also make sense to further prefix it with something that shows that those ids are not part of the layouting structure, but form data related.
E.g. currently a field "content" might conflict with a layout id #content for a div container containing the content body.
If we prefixed those form related ids with data- this would also make the distinction clearer.

Together with @DSchalla s idea it could be:

data-{entity}-{field}

or something like that.

The branding/word "cake" I would try to keep out of it, though :)

@DSchalla
Copy link
Author

Yeah I like the idea, something like that would make sense imho. Maybe instead of data use form to make clears its related to an form of this entity, but sounds great otherwise.

@markstory
Copy link
Member

If you know you will have collisions, why not just set the idPrefix. The helper will not always be able to guess the prefix which is why it doesn't try.

@markstory markstory modified the milestones: 3.1.0, 3.0.0 Mar 22, 2015
@DSchalla
Copy link
Author

Set the ID Prefix for every form instead of just setting an default prefix which makes it much more unlikely overcomplicates things, don't you think so? Sure, I could add a form prefix in about 30 forms, but that seems way more complicated than adding just a default prefix which avoids that trouble for multiple people, since I cannot see a disadvantage of that Enhancement at all.

The helper cannot guess the prefix, of course, but it can have an default prefix which avoids generic ids like title, id, content, created to be existent twice in an system. There is not any drawback, since the prefix can still be overwritten in the helper call.

@markstory
Copy link
Member

Is the problem that you are getting collisions between forms, or between inputs and layout elements?

@DSchalla
Copy link
Author

Well, HTML ids have to be unique, and when I got an div#content and an entity property content, this leads to the collision. So basically between input and layout elements, but it could be anything else.

@markusramsak
Copy link
Contributor

You can also get collisions when using forms loaded via ajax in a popup for example. I solved this uniqueness problem by setting the id prefix to "Text::uuid()" in Form->create().

@dereuromark
Copy link
Member

I would rather have a more basic prefix like "data" again, as it was in 2.x. Everyone else can still make it more unique if he needs to. The current collisions (by default), though, are not a good thing to keep either way.

@markstory
Copy link
Member

There was no generic id prefix in 2.0 @dereuromark. I think @jabd's pull request is headed in the right direction though.

@dereuromark
Copy link
Member

@markstory The model name was that prefix afaik.

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

Successfully merging a pull request may close this issue.

4 participants