Fix: #127 - generate unique ID upon creation

[torleif]

Fixes front end components that require a unique ID. Duplicate IDs in the DOM is invalid HTML.

[helpfulrobot]

@torleif, thanks for your PR! By analyzing the blame information on this pull request, I identified @wilr to be a potential reviewer

[stevie-mayhew]

Can you move the comment above the line? The indentation on this also needs to be fixed up to match the rest of the code style

[torleif]

Thanks for the feedback, I'll have to get into the habit of using spaces instead of tabs!

[stevie-mayhew]

I'd be happy to merge this as it seems like a reasonable fix, but there is also a lot of handling for "new-" components already in place. If we make this change we should remove the "new-" handling from the code base and rely on the fact we always have an instantiated object.

However I feel like it might be better to not save these at this point and instead change the way we handle the IDs for the interior elements so that they reflect the "new" state. @dhensby any thoughts here?

[torleif]

@stevie-mayhew it looks like there's some JavaScript that uses maxid to work out the next id of the widget by referencing "new-". This could be dangerous as if two people create a widget at the same.

I'll try and figure out what this javascript does

[torleif]

From what I can tell WidgetAreaEditor.js checks if it's a new element and replaces the ID of what it thinks it is going to be. Though pretty poorly given #127 creates incorrect IDs for textareas.

The JavaScript doesn't break with the code given that there's no new elements, though it can be tidied up by removing the references of "new-":

            insertWidgetEditor: function(response) {
                $('#usedWidgets-'+$(this).attr('name')).append(response);
                this.rewriteWidgetAreaAttributes();
            },

Let me know if you want me to put this in the PR

[stevie-mayhew]

If nobody else has any objections I'm going to merge this later today.

[dhensby]

The risk here is if validation is on for the object and that fails, then no one can ever start creating a widget again...

[torleif]

@dhensby I've added a validation check before writing the dataobject. I guess there would be some people who don't want a blank DataObject written for validation purposes, but note this would enable duplicate IDs if the CMS author creates two or more objects that can't be blank.

[dhensby]

this feels so nasty...

@tractorcow any thoughts?

---

NB: this is why using increment-ing DB IDs is not a good idea for models :(

[torleif]

A tidier solution would be to throw an exception if the blank object is invalid. This would prevent duplicate IDs and force the developer to use widgets that can't be blank.

Thoughts?

[tractorcow]

The trouble is an empty widget perhaps isn't valid... thus it could become impossible to create a valid widget in any context.

Have you explored perhaps dis-entangling HTML form ID from the dataobject ID? Why does it need to be that field, rather than say an automatically generated FormID property that can default to a uniqid() ?

That way it can exist (and be stored in a hidden field) prior to being written to the database.

[tractorcow]

In Widget::CMSEditor() you can do something like

// ...
$this->FormID = $this->FormID ?: uniqid();
$outputFields->push(new HiddenField('Widget[' + $this->FormID + '][FormID]', $this->FormID));
foreach ($fields as $field) {
    // ...
    $name = preg_replace("/([A-Za-z0-9\-_]+)/", "Widget[" . $this->FormID . "][\\1]", $name); 
    // ...
}

[tractorcow]

Redundant check in any case... DataObject observably has a doValidate method :)

[torleif]

@tractorcow thanks, I've made a patch to do something similar to what you've suggested. Works as expected in MSIE, FFx and Chrome.

[tractorcow]

How necessary is the ->write() that you've added? We still have the problem of writing invalid records before they are populated with content.

E.g. widgets that fail validation if Title is empty.

[torleif]

@tractorcow sweet - I didn't reset the patch before committing. ops.

[dhensby]

Great, please can you squash the commits and check the indentation around CMSEditor declaration? then we are good to merge

[torleif]

@dhensby sweet, rebase has been completed and works as expected

[dhensby]

thanks

[tractorcow]

Good job @torleif