[META] Widgets as Plugins [#1742734]

Comment	File	Size	Author
#51	field-plugins-widgets-1742734-51.patch	134.97 KB	yched
#50	field-plugins-widgets-1742734-50.patch	134.78 KB	yched
#49	field-plugins-widgets-1742734-49.patch	134.86 KB	yched
#46	field-plugins-widgets-1742734-46-4.patch	121.09 KB	yched
#43	field-plugins-widgets-1742734-43.patch	117.16 KB	yched
#42	field-plugins-widgets-1742734-42.patch	113.86 KB	yched
#40	field-plugins-widgets-1742734-40.patch	113.83 KB	yched
#38	field-plugins-widgets-1742734-38.patch	112.04 KB	yched
#31	field-plugins-widgets-1742734-31.patch	103.19 KB	yched
#28	field-plugins-widgets-1742734-28.patch	102.86 KB	yched
#4	1742734.Widgets-as-plugins.patch	732 bytes	jerdavis

Comment #1

yched CreditAttribution: yched commented 21 August 2012 at 14:52

Actually, changed the branch name so that it contains the issue #

Log in or register to post comments

Comment #2

yched CreditAttribution: yched commented 21 August 2012 at 15:03

Note Field UI currently still has quirks - working on it.

Log in or register to post comments

Comment #3

KarenS CreditAttribution: KarenS commented 22 August 2012 at 21:18

Notes from my first review:

- It would be helpful to have a diff from HEAD to look at.

- I know we talked about keeping a compatibility layer and some old code to pass tests, but this actually makes it quite hard to review. For instance, I see the new form handling in the widget plugin, but I still see field.default.inc and field.form.inc. I think for review it would be easier to see that the form handling used to be here is now gone and then go see where it has gone to. Seeing it in both places is confusing, even when I know why it is there.

- Is the code actually supposed to work to create a new installation and then create a content type and add a text field? If so, it does not quite. When I create the text field I get the message "Notice: Indirect modification of overloaded element of Drupal\field\Field\FieldInstance has no effect in field_default_form() (line 68 of core/modules/field/field.form.inc)." This message shows up when the field is created and then persistently on the field edit page but no where else. When I try to add a node using the field, the value is saved, but my title is replaced by the work 'Error'.

- In the widget plugin form() function, we are passing $entity and $entity_type to the form. Didn't we already know those things when the plugin was instantiated? Should we just add those values to the plugin and refer to them later? The same may be true for $language, but I don't know if there is any reason why $language might vary later.

- The functions obviously need more documentation. As I and others review this we can start adding that documentation as we develop an understanding of how it is supposed to work, then you can review the documentation to confirm that we got it right. By the time we have a patch for other reviewers, hopefully the documentation will make good sense.

- For the problem of reviewing changes that contain both the new and old code in order to not break tests while we get feedback on the API, what about this idea? Remove the old code and create a few widgets as you were planning to do, knowing that other widgets are now broken. Post the patch on d.o. with the provision that it won't pass tests but the approach can be tested by creating a new site that uses only the functional widgets. If you get agreement on the general API, come back and create the rest of the widgets so the tests pass again. Having tried to review this myself with the mixture of old and new code, I'm afraid no one else will be able to make sense of it if it includes code that doesn't really belong there.

- I really like the direction, this code will be so much cleaner and easier to understand and extend!

Log in or register to post comments

Comment #4

jerdavis

Minneapolis, MN

CreditAttribution: jerdavis commented 23 August 2012 at 16:36

Status:

Active

» Needs review

File	Size
1742734.Widgets-as-plugins.patch	732 bytes

- Is the code actually supposed to work to create a new installation and then create a content type and add a text field? If so, it does not quite. When I create the text field I get the message "Notice: Indirect modification of overloaded element of Drupal\field\Field\FieldInstance has no effect in field_default_form() (line 68 of core/modules/field/field.form.inc)." This message shows up when the field is created and then persistently on the field edit page but no where else. When I try to add a node using the field, the value is saved, but my title is replaced by the work 'Error'.

Attached patch should resolve, offsetGet() needs to return by reference, see here for some more background on this type of issue:

http://drupal.org/node/1717186#comment-6382254

Log in or register to post comments

Comment #5

KarenS CreditAttribution: KarenS commented 24 August 2012 at 08:28

Status:

Needs review

» Needs work

Hmm, that change gives me a ton of messages like:

Notice: Only variable references should be returned by reference in Drupal\field\Field\FieldInstance->offsetGet() (line 59 of core/modules/field/lib/Drupal/field/Field/FieldInstance.php).

I'm using PHP 5.3.6. According to your link, that version should have relaxed standards on this, but doesn't seem to.

It does get rid of the original error message though.

The second problem I mentioned, that the title is changed to 'Array', is actually not a problem with the value that is saved, it's a problem with the display of the title, so I don't know where that is coming from.

Log in or register to post comments

Comment #6

KarenS CreditAttribution: KarenS commented 24 August 2012 at 08:34

Status:

Needs work

» Active

OK, I removed that change and did a git pull and I'm no longer getting the original error about overloading, so something seems to have been fixed since I first reported it, without this patch.

Log in or register to post comments

Comment #7

KarenS CreditAttribution: KarenS commented 24 August 2012 at 08:45

Status:

Active

» Needs work

Ugh, sorry for the noise. I forgot where the original error message was, it is still there. But this patch doesn't seem to be the solution.

Log in or register to post comments

Comment #8

KarenS CreditAttribution: KarenS commented 24 August 2012 at 08:50

I ran into another error trying to use a simple text widget and posted it in a separate issue #1749834: Widget Add More error

Log in or register to post comments

Comment #9

yched CreditAttribution: yched commented 24 August 2012 at 10:22

I'm working on the Field UI breaks, but it ties down to field crud, which is not our cleanest code right now. I'll post an update when it's done

Log in or register to post comments

Comment #10

jerdavis

Minneapolis, MN

CreditAttribution: jerdavis commented 24 August 2012 at 11:25

@karens

The message:

Notice: Only variable references should be returned by reference in Drupal\field\Field\FieldInstance->offsetGet() (line 59 of core/modules/field/lib/Drupal/field/Field/FieldInstance.php).

Is a result of the conditional statement in the return. This could be rewritten something like this:

  /**
   * Implements ArrayAccess::offsetGet().
   */
  public function &offsetGet($offset) {
    if(isset($this->definition[$offset])) {
      return $this->definition[$offset];
    }
    else {
      return NULL;
    }
  }

Or, potentially just return $this->definition[$offset]. In testing locally that did not cause any errors, though I don't know if there would be a condition where $offset didn't exist and therefore triggered an undefined index error.

  /**
   * Implements ArrayAccess::offsetGet().
   */
  public function &offsetGet($offset) {
    return $this->definition[$offset];
  }

Thoughts? I can update a patch for either.

Log in or register to post comments

Comment #11

yched CreditAttribution: yched commented 24 August 2012 at 11:47

Status:

Needs work

» Active

Pushed the fixes for Field UI. The add / edit process should work, but default values are not saved yet.

Log in or register to post comments

Comment #11.0

yched CreditAttribution: yched commented 24 August 2012 at 11:47

Issue summary:

[META] Widgets as Plugins

Comments