Edit : Patch is now posted in the core queue: #1785256: Widgets as Plugins
Let's start with widgets only.
Code is in branch field-plugins-widgets-1742734
Core issue used for testbot runs (until we have a patch ready to push for review in the wild):
#1777218: Testbot helper issue for 'Widgets as Plugins"
Subtasks :
#1781250: Run performance tests
#1778818: Convert test widgets
#1774254: Switch to annotation-based discovery
#1762828: Figure out hook_field_widget_properties_alter()
#1774316: Account for new "weight' property in hook_field_widget_info()
#1761030: Lose the entity_type parameter when the entity is available
#1753602: Create a LegacyWidget plugin for BC with old-style widgets
#1744216: Implementation of Number widgets
#1751020: Rename BaseWidget to WidgetBase
Followups:
#1762802: Create a custom WidgetFactory
#1751164: Implement the test_field_widget_multiple widget as a Plugin
#1751156: Implement the test_field_widget widget as a Plugin
#1751174: Convert file and image module widgets / formatters to Plugin system
#1751186: Convert image module widgets to Plugin system
#1770172: Convert email module widgets and formatters to Plugin system
#1751232: Implement the options_onoff widget as a plugin
#1751234: Convert option widgets to Plugin system
#1751236: Implement the options_select widget as a Plugin
#1751244: Convert taxonomy module widgets to Plugin system
Upstream blockers:
#1751100: Bump minimum version of php required to 5.3.5 - our use of ArrayAccess requires PHP 5.3.4 [UPDATE: core requirements haven't been actually bumped yet, but the testbots now run 5.3.4, so we can at least run tests in the core queue, which is good enough for now]
Upstream issues we currently circumvent in our code:
#1764232: CacheDecorator provides no way to clear cached definitions
#1764278: Run PluginManagerBase::processDefinition() in a ProcessDecorator
#1778942: Discovery::getDefinition() / getDefinitions() : inconsistent return values for "no result"
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 |
Comments
Comment #1
yched CreditAttribution: yched commentedActually, changed the branch name so that it contains the issue #
Comment #2
yched CreditAttribution: yched commentedNote Field UI currently still has quirks - working on it.
Comment #3
KarenS CreditAttribution: KarenS commentedNotes 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!
Comment #4
jerdavisAttached 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
Comment #5
KarenS CreditAttribution: KarenS commentedHmm, 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.
Comment #6
KarenS CreditAttribution: KarenS commentedOK, 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.
Comment #7
KarenS CreditAttribution: KarenS commentedUgh, 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.
Comment #8
KarenS CreditAttribution: KarenS commentedI ran into another error trying to use a simple text widget and posted it in a separate issue #1749834: Widget Add More error
Comment #9
yched CreditAttribution: yched commentedI'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
Comment #10
jerdavis@karens
The message:
Is a result of the conditional statement in the return. This could be rewritten something like this:
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.
Thoughts? I can update a patch for either.
Comment #11
yched CreditAttribution: yched commentedPushed the fixes for Field UI. The add / edit process should work, but default values are not saved yet.
Comment #11.0
yched CreditAttribution: yched commentedbetter branch name
Comment #12
yched CreditAttribution: yched commented- Pushed fixes for 'default values not saving on field config form"
- merged @tstoeckler's #1751020: Rename BaseWidget to WidgetBase
- Turning this issue into a meta issue
Comment #13
yched CreditAttribution: yched commentedComment #14
yched CreditAttribution: yched commented48a89be : renamed FieldInstance::widget() to FieldInstance::getWidget() :
Comment #14.0
yched CreditAttribution: yched commentedAdding substasks
Comment #14.1
tstoecklerAdded links to all the (postponed) conversion tasks.
Comment #15
yched CreditAttribution: yched commented7fb08109 : merged @tstoeckler changes (add missing function to WidgetInterface, and move PHPdocs over to there)
Comment #16
yched CreditAttribution: yched commentedre: @KarenS's remarks in #3 (discussed with her during the Munich code sprint)
- The exact BC strategy does indeed still need some strategy discussion. We should try to reach out to catch and/or webchick for feedback on the best strategy.
- We had a discussion on the signatures of the PluginInterface methods that are invoked through _field_invoke (which right now enforces a common func signature on the target methods). We concluded that :
$entity makes sense as a parameter, since the same Widget object can potentially be used on several $entities within a request (entities of the same bundle, where the same instance appears). Thus is it valid to have it as "injected in each method".
$entity_type can now be derived from $entity, but removing those explicit $entity_type params throughout core is a task for a separate patch. Same goes for $langcode.
$field and $instance should be part of the Widget object properties (injected in the constructor).
Thus, committed:
44aa8eb : remove $field and $instance as params in methods invoked through _field_invoke()
Comment #17
yched CreditAttribution: yched commentedDiscussed strategy with webchick, sun, EclipseGc:
- webchick totally approves the "start with only 1 or 2 widgets converted", both for an initial reviewable patch but also possibly for a first committed patch.
- possible ways to go with this approach and not break tests (sorted by "preferred option ASC")
a) comment out tests :-/,
b) find a way to keep the old field_default_form_*() functions + hook_field_widget_*() callbacks working along next to the new WidgetInterface plugin objects. The code "to be eventually deleted but kept around for now" would be moved to a field.legacy.inc file. This brings more clarity for reviewers, and also makes sure we get conflicts when merging upstream changes that affect the old code, which is a good notification to have to make sure we don't miss any upstream change when we rm the old code.
This relies on making _field_invoke() be able to call both old style and new style widget code. fubhy has ideas about how we could do this, but we couldn't synchronize so far.
c) come up with a LegacyWidget plugin class that acts as an adapter and just calls the old callbacks. field_default_form_*() is gone.
I'll try to investigate c).
Comment #18
yched CreditAttribution: yched commentedCreated #1753602: Create a LegacyWidget plugin for BC with old-style widgets, and updated the issue summary
Comment #18.0
yched CreditAttribution: yched commentedAdded the taxonomy issue, I forgot before
Comment #19
yched CreditAttribution: yched commentedPushed a couple commits from Munich sprint days that were lagging on my local repo.
- 3cf4d75 : remove $field and $instance from the params passed to methods invoked through _field_invoke() ($field and $instance are injected in the Widget's contructor, they don't need to be additionally injected in each method)
- b6ed065 : Split WidgetInterface into :
- WidgetBaseInterface : the wrapping methods that contain the nasty generic logic that you probably do not want to override (although you can).
- WidgetInterface (extends WidgetBaseInterface): the actual (simple) methods that you need to implement in order to be a Widget
No impact on actual Widgets code, but very elegantly solves the documentation issue. Credits @EclipseGc for the idea.
Comment #20
yched CreditAttribution: yched commentedPushed :
- cb4b1d7 : move deprecated hook_field_widget_*() from field[_ui].api.php to WidgetInterface.php
- e8fc58d : fix obsolete handling for WidgetBase::form() being called with $entity == NULL
Comment #21
yched CreditAttribution: yched commentedPushed :
6ca2c9d : Actually call the Widget methods for 'extractFormValues' and 'submit', instead of the old field_default_* hooks.
Doh. Sprints are nice, but they also can make you commit half-assed code :-(
Comment #21.0
yched CreditAttribution: yched commentedAdded issue
Comment #22
yched CreditAttribution: yched commentedAdded #1761030: Lose the entity_type parameter when the entity is available to the issue summary.
Comment #23
yched CreditAttribution: yched commentedAlso, FYI : the LegacyWidget in #1753602: Create a LegacyWidget plugin for BC with old-style widgets works absolutely great ! Thanks @Stalski & @zuuperman !
Some last cleanup and we can merge it back to the main branch.
So #17 above is fixed : we have a great way to attack the core queue with a moderate-size patch that has a chance to turn green :-)
Comment #23.0
yched CreditAttribution: yched commentedadd subtask
Comment #23.1
yched CreditAttribution: yched commentedadded blocker
Comment #24
yched CreditAttribution: yched commentedMerged #1753602: Create a LegacyWidget plugin for BC with old-style widgets (field-plugins-widgets-legacy-1753602).
Comment #25
Stalski CreditAttribution: Stalski commentedThis is fantastic news. Great job yourself!
Comment #26
yched CreditAttribution: yched commented- Merged in Stalki's #1761030: Lose the entity_type parameter when the entity is available
- Merged latest 8.x HEAD
Comment #27
yched CreditAttribution: yched commentedPushed ae94cb2 : Removed the code from #1040790: _field_info_collate_fields() memory usage.
We can actually work on top of the current _field_info_collate_fields(). Better to keep the changes size small when we go to the core queue.
Comment #28
yched CreditAttribution: yched commentedPushed 35654c1 : cleanified PluginSettingsBase a bit, added missing phpdocs and an interface
Also, attached is a patch against 8.x, to get a better overview of the changes.
Comment #28.0
yched CreditAttribution: yched commentedreorder tasks
Comment #29
yched CreditAttribution: yched commentedCreated #1762802: Create a custom WidgetFactory, added in the summary (not critical).
Comment #30
yched CreditAttribution: yched commentedCreated #1762828: Figure out hook_field_widget_properties_alter(), and added in the summary.
Comment #30.0
yched CreditAttribution: yched commentedadded subtask
Comment #30.1
yched CreditAttribution: yched commentedadd subtask
Comment #30.2
yched CreditAttribution: yched commentedreorder tasks
Comment #30.3
yched CreditAttribution: yched commentedAdded http://drupal.org/node/1764232
Comment #30.4
yched CreditAttribution: yched commentedReorganize tasks
Comment #31
yched CreditAttribution: yched commentedPushed some cleanups :
- 6aaee8b : we forgot hook_field_widget_settings_form() in the LegacyWidget
- 7d83d73: Renamed WidgetInterface::fieldValuesFromFormValues() to WidgetInterface::massageFormValues()
- 51ee96c; Update and simplify validation / submit process for the current behavior of EntityFormController. Removes WidgetInterface::submit()
Updated patch against 8.x attached.
Comment #31.0
yched CreditAttribution: yched commentedAdd 1764278
Comment #32
Stalski CreditAttribution: Stalski commentedThe tests keep giving an error on hook_field_widget_form_alter implementation. It expects "field" to be set.
Error is:
I saw this when working on #1762828: Figure out hook_field_widget_properties_alter(). In the branch for that issue, I fixed it to have clean tests ... .
for this branch, committed it as well. It seems to me it's a straight forward fix, since the code that uses it now works properly (as intended imo).
Comment #33
yched CreditAttribution: yched commented@Stalski, indeed, that's wrong copy/paste from my "all inclusive" code in field-plugins-yched.
Thks for the fix - I moved the line up a bit where it used to be in HEAD.
Comment #33.0
yched CreditAttribution: yched commentedAdded email widget type conversion task
Comment #34
yched CreditAttribution: yched commentedCreated #1774254: Switch to annotation-based discovery :-)
Comment #34.0
yched CreditAttribution: yched commentedAdded subtask
Comment #35
yched CreditAttribution: yched commentedCreated #1774316: Account for new "weight' property in hook_field_widget_info()
Comment #36
yched CreditAttribution: yched commented- Merged #1762828: Figure out hook_field_widget_properties_alter()
- Merged current 8.x
Comment #37
yched CreditAttribution: yched commentedPushed:
- f2f79cd #1774316: Account for new "weight' property in hook_field_widget_info()
- 9f0f277 Fix for fatal error on 'instance edit form' for taxo fields
Comment #37.0
yched CreditAttribution: yched commentedAdded subtask
Comment #37.1
yched CreditAttribution: yched commentedAdded testbot issue
Comment #38
yched CreditAttribution: yched commentedCreated #1777218: Testbot helper issue for 'Widgets as Plugins" to have core testbot runs until we have a patch ready to push for review in the wild.
Also, current state of the patch attached, for those interested.
Comment #39
yched CreditAttribution: yched commentedMerged #1762802: Create a custom WidgetFactory
Comment #40
yched CreditAttribution: yched commentedMerged latest HEAD, and pushed :
- 4617e35: _field_info_prepare_instance_widget() is not needed anymore. Preparation happens at run-time, only when needed
- f4f0aea Fix 'Undefined index: weight' in tests.
Comment #41
yched CreditAttribution: yched commentedCreated #1778818: Convert test widgets
Comment #41.0
yched CreditAttribution: yched commentedwording
Comment #41.1
yched CreditAttribution: yched commentedAdd subtask
Comment #41.2
yched CreditAttribution: yched commentedNote about testbots & PHP 5.3.4
Comment #41.3
yched CreditAttribution: yched commentedFormatting
Comment #42
yched CreditAttribution: yched commentedPushed :
- d4cadc7 Fixed forum test fails
Comment #43
yched CreditAttribution: yched commentedPushed fixes for a bunch of test failures :
- cc7e1cf
- 60e54b6
- a9cc1c2
- bd33580
Comment #43.0
yched CreditAttribution: yched commentedremove mention of old branch name
Comment #44
yched CreditAttribution: yched commentedPushed :
- 31857c1:fix FieldInfoTest failures
- 3b73859: rename WidgetInterface::extractFormValues() to submit()
- db37449: fix error when 'default value' input has field validation errors
Incidentally - we're GREEN !! : #1777218-9: Testbot helper issue for 'Widgets as Plugins"
Comment #45
yched CreditAttribution: yched commentedPushed
- 0e303ab: introduce separate field_invoke_method(), leave the legacy _field_invoke() alone
- a couple phpdoc / comment fixes
Comment #46
yched CreditAttribution: yched commentedAnd here's the updated patch.
Comment #46.0
yched CreditAttribution: yched commentedAdded upstream issue
Comment #47
yched CreditAttribution: yched commentedCreated #1781250: Run performance tests, and added it to the issue summary.
Comment #48
yched CreditAttribution: yched commentedPushed :
- cf4e065: Merged #1778818: Convert test widgets - Thanks Stalski !
Comment #49
yched CreditAttribution: yched commentedPushed :
- 91f2088: Merge #1774254: Switch to annotation-based discovery - Yay !, & kudos to Stalski again :-)
Comment #50
yched CreditAttribution: yched commentedPushed :
- Merge latest 8.x
- e6468fe Move LegacyDiscoveryDecorator.php next to WidgetFactory.php
I think we're in good shape now. Working on an issue summary to post to the core queue :-)
Comment #51
yched CreditAttribution: yched commentedPushed:
- 642b53e Cleanup the folder/namespace organisation below field/lib/Drupal/field.
Old folder structure : http://privatepaste.com/download/0e0cf585f8
New folder structure : http://privatepaste.com/download/e947359d81
Comment #52
yched CreditAttribution: yched commentedPushed a couple more cleanups (mostly comments) and... posted a patch to the core queue :-)
#1785256: Widgets as Plugins
Closing this one as a duplicate. See you guys over there !
Comment #52.0
yched CreditAttribution: yched commentedAdded subtask
Comment #52.1
yched CreditAttribution: yched commentedLink to core patch