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

FIX: Ensure that ChangeTrackerOptions doesn't get overriden #125

Merged
merged 1 commit into from
Dec 19, 2013
Merged

FIX: Ensure that ChangeTrackerOptions doesn't get overriden #125

merged 1 commit into from
Dec 19, 2013

Conversation

nedmas
Copy link
Contributor

@nedmas nedmas commented Nov 19, 2013

Use _super() to reference parent getter in LeftAndMain.EditForm.js to ensure that ChangeTrackerOptions is modified and not overriden.

See #118 for initial bug report.

@hafriedlander
Copy link

Hi. Sorry, I was going to have a look at this on the back of that issue @chillu raised but you beat me to it. There's a couple of edge cases that aren't obvious that come from ChangeTrackerOptions being an object, and might need an Entwine API extension to fix nicely.

Objects in entwine properties are a bit dangerous, because javascript always passes them by reference instead of cloning them. Entwine also doesn't clone them when using them as default values.

The result is that this patch will repeatedly add that selector to the result every time getChangeTrackerOptions is called, so it'll be there once the first time it's called, twice the second, etc.

The right fix at the moment would look like:

$('.cms-edit-form').entwine({
  getChangeTrackerOptions: function() {
    // Figure out if we're still returning the default value
    var isDefault = (this.entwineData('ChangeTrackerOptions') === undefined);
    // Get the current options
    var opts = this._super();

    if (isDefault) {
      // If it is the default then...
      // clone the object (so we don't modify the original),
      var opts = $.extend({}, opts);
      // modify it,
      opts.ignoreFieldSelector +=', input[name=IsSubsite]';
      // then set the clone as the value on this element 
      // (so next call to this method gets this same clone)
      this.setChangeTrackerOptions(opts);
    }

    return opts;
});

This is super ugly though, non-obvious, and could maybe be handled better in the entwine layer.

@nedmas
Copy link
Contributor Author

nedmas commented Nov 20, 2013

Thanks @hafriedlander that's a really useful insight into the way entwine works. I hadn't even considered that the selector would be added every time. I understand that your fix wouldn't be the best long term solution but would it be ok to incorporate into this PR so we have a fix for the time being? @chillu what do you think?

@nedmas
Copy link
Contributor Author

nedmas commented Dec 3, 2013

Ok so I've update the PR with the code from @hafriedlander any chance this can get merged in @chillu?

@nedmas
Copy link
Contributor Author

nedmas commented Dec 12, 2013

I don't wish to be a pain, but it would be really nice to get this merged in or at least get an update as to why it's not been merged already. I understand if it's just a time thing, and like I said I don't wish to be a pain just thought I'd ask.

@mateusz
Copy link
Contributor

mateusz commented Dec 16, 2013

Oh man this indeed is hacky :-)

I'm happy to merge this in, but could you put Hamishes dissertation into the commit message, plus a link to this PR discussion? Also maybe put a link to this discussion into the code itself - this is a temporary measure and would be great if we make it clear.

From @hafriedlander:
Hi. Sorry, I was going to have a look at this on the back of that issue @chillu raised but you beat me to it. There's a couple of edge cases that aren't obvious that come from ChangeTrackerOptions being an object, and might need an Entwine API extension to fix nicely.

Objects in entwine properties are a bit dangerous, because javascript always passes them by reference instead of cloning them. Entwine also doesn't clone them when using them as default values.

The result is that this patch will repeatedly add that selector to the result every time getChangeTrackerOptions is called, so it'll be there once the first time it's called, twice the second, etc.

The right fix at the moment would look like:
```php
$('.cms-edit-form').entwine({
  getChangeTrackerOptions: function() {
    // Figure out if we're still returning the default value
    var isDefault = (this.entwineData('ChangeTrackerOptions') === undefined);
    // Get the current options
    var opts = this._super();

    if (isDefault) {
      // If it is the default then...
      // clone the object (so we don't modify the original),
      var opts = $.extend({}, opts);
      // modify it,
      opts.ignoreFieldSelector +=', input[name=IsSubsite]';
      // then set the clone as the value on this element
      // (so next call to this method gets this same clone)
      this.setChangeTrackerOptions(opts);
    }

    return opts;
});
```
This is super ugly though, non-obvious, and could maybe be handled better in the entwine layer.

See #125
mateusz added a commit that referenced this pull request Dec 19, 2013
FIX: Ensure that ChangeTrackerOptions doesn't get overriden
@mateusz mateusz merged commit d21c92a into silverstripe:master Dec 19, 2013
@mateusz
Copy link
Contributor

mateusz commented Dec 19, 2013

Yeah, thanks that will do :-)

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

3 participants