This is a list to track issues to cleanup Field API procedural functions and replace them with new API. Most issues already have new API in place. Some might not, but in light of doing cleanups, it might be nice to clean those up too.

These are the ones that given the time we still have are feasible to get done. We still also have the BC layer that needs to be removed too, see #1953408: Remove ArrayAccess BC layer from field config entities. After all this stuff, we'll have a pretty damn modern Field API.

See also #2067127: Reorganize the contents of field.* files for a file reorg to get a clearer view of the current status & contents of field.module

CommentFileSizeAuthor
#11 field_deprecated-2061107-11.patch13.51 KByched
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

Issue tags: +Field API

tagging

xjm’s picture

Priority: Normal » Major
Issue tags: +API change, +API clean-up

So there are basically three parts to this for each of these issues:

  1. Deprecate the procedural functions.
  2. Clean up core to not use the procedural functions.
  3. Remove the procedural functions.

#3 is the part that will need to be evaluated by a core maintainer to consider whether it can be allowed after API freeze. My thought is that there is a lot of value in removing the D7 API.

  • We announced after the original CMI conversion that #1953408: Remove ArrayAccess BC layer from field config entities would happen, and that along with all the entity API changes represent a significant enough API break that there is little value in retaining these procedural wrappers for BC.
  • It would be confusing, possibly even bewildering, for developers (especially developers new to D8) to see an entire subsystem stuffed with this unused, deprecated cruft.
  • As a field module maintainer, finding what I needed in the D7 API was difficult and frustrating. I feel that the D8 API has gone a long way to improving the DX and making the API more self-explanatory. Some of this advantage is lost if developers still has to wade through a sea of field_whatever_() on api.d.o to find what they need.
  • Retaining the procedural wrappers will hinder the adoption of best practices like dependency injection.
  • Wrappers or no, it's a whole lot of code we'd still have to maintain and support for no good reason. We're not just talking about one or two functions; we're talking about an entire API that's already dependent on things that are being completely overhauled: the entity system, entity and field storage, and the configuration system.

All that said, @swentel pointed out (and I agree) that these issues are less important than solving critical issues like:

...so if we need to make tough calls about what to prioritize to get the Field API done, these issues will have to take a back seat to those, and we should avoid disrupting those patches where possible.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Title: [Meta] Cleanup procedural functions in Field API » [Meta] Clean up deprecated procedural functions in Field API

Tag monster--

yched’s picture

There are also the field.multilingual.inc functions, on which the way forward is unclear to me :-)
Opened #2067079: Remove the Field Language API, added it to the issue summary.

yched’s picture

Issue summary: View changes

Updated issue summary.

yched’s picture

Also, posted a patch in #2067127: Reorganize the contents of field.* files, to get a clearer view of the current status & content of field.module.

yched’s picture

xjm’s picture

For this issue, @Dries suggested that things that are already marked @deprecated and simply wrap the new implementations can be removed. For those that don't yet have a replacement, we can add the replacement, but should probably retain the BC wrappers. We also agreed that these cleanups are less important than #1953408: Remove ArrayAccess BC layer from field config entities and other critical field API patches, and so we might do best to resolve those issues first to avoid disruption from these changes.

yched’s picture

Agreed that #1953408: Remove ArrayAccess BC layer from field config entities is more important.
Would be good if we could get #2067127: Reorganize the contents of field.* files in soonish, would make those things easier to sort out.

yched’s picture

webchick’s picture

Title: [Meta] Clean up deprecated procedural functions in Field API » Remove deprecated procedural functions in Field API
Issue summary: View changes
Status: Active » Postponed
Issue tags: +beta blocker, +DX (Developer Experience)

A case has been made by several folks that keeping these old functions around represents a big enough learnability challenge for the new Entity/Field API that we should most likely clean this up prior to beta.

I'd like to do this as one patch, however, to remove the entirety of field.deprecated.inc, once those functions all actually have OO replacements that are used throughout core. That avoids overwhelming the RTBC queue with a bunch of issues like #2155697: Remove a couple simple deprecated functions (and their associated review/re-roll cycle hell), and also keeps focus on the things that matter (making sure those new OO subsystems exist and are complete and functional), with this being a nice cherry on top of the sundae at the end once we've done all the hard work.

So: tagging, postponing, un-metaing.

Berdir’s picture

Agreed that an issue like #2155697: Remove a couple simple deprecated functions doesn't make much sense, but what about something like #2157153: Remove field_attach_preprocess()? It removes usage of that function and removes it as well.

Field API includes a ton of (deprecated) functions over a wide range of things, language related, preprocessing, widget/formatter handling, accessing field values and so on. Having a separate issue to remove the usage and mark it deprecated, then remove it here seems actually more work to me, removing both usage and the functions in here IMHO makes harder to review and violates http://webchick.net/please-stop-eating-baby-kittens.

Some of the "deprecated" functions don't even have a replacement yet, there again makes it more sense to me to remove that issue in the issue that provides the replacement, e.g. to prove that nothing is remaining that's still using it.

Apart from the mentioned issue which randomly removes some functions that happen to be not used anymore and leaves others, this seems like a different scenario than the hasPermission() or unused variables meta issues to me.

yched’s picture

Status: Postponed » Needs review
FileSize
13.51 KB

@webchick:
Unfortunately, it's not that simple :-/
Here's a rundown on where we stand wrt field.deprecated.inc:

The following have replacements and are not used in core anymore. They can be removed any time.

- field_info_field_types()        --|
- field_info_field_settings()       |
- field_info_instance_settings()    |
- field_info_widget_types()         |--> plugin managers
- field_info_widget_settings()      |
- field_info_formatter_types()      |
- field_info_formatter_settings() --|

- field_read_field()      --|
- field_read_fields()       |--> entity_load_multiple_by_properties() / EFQ
- field_read_instance()     |
- field_read_instances()  --|

- field_get_items()         --> Entity translation API
- field_get_default_value() --> Entity Field API
- field_access()            --> Entity Field API

For the rest:

Field registry :

- field_info_field_map()
- field_info_field()
- field_info_field_by_id()
- field_info_field_by_ids()
- field_info_instances()
- field_info_instance()

The final API depends on the outcome of #2116363: Unified repository of field definitions (cache + API), so there's little point in forcing people to move now, the replacements we would point them to might be obsolete in a couple weeks.

Field attach * :

- field_attach_form()
- field_attach_form_validate()
- field_attach_form_submit()
- field_attach_prepare_view()
- field_attach_view()

@deprecated is a lie, we currently don't have replacements for those, and current core still calls them. That's what #2095195: Remove deprecated field_attach_form_*() is about.

- Then, as @Berdir points, there's field_attach_preprocess(), which is *not* marked as deprecated and is still called, but is actually totally useless with Twig.
#2157153: Remove field_attach_preprocess() simply removes it, without needing any other code or test adjustment.

----------------

In short: the functions we can't remove right now have their own critical but thorny issues, I don't think it's reasonable to wait on them. I'd plea that we remove what can be removed now - honestly, getting them out of our mental plate would help.

Attached patch takes care of the ones listed at the top, and #2157153: Remove field_attach_preprocess() is about field_attach_preprocess().

xjm’s picture

That's very helpful information. Thanks a ton @yched.

xjm’s picture

Quick followup: So the first group could be removed in one patch here, the second group maybe deserves its own postponed issue, and the third group probably needs one or multiple issues blocked on #2095195: Remove deprecated field_attach_form_*()?

Can we file issues for the second and third things (one for group B, one for group C) if they don't exist yet, tagged as beta blockers and postponed on the appropriate blocking issues?

xjm’s picture

Issue tags: +Approved API change

Also, @webchick already approved ripping out all these deprecated wrappers (not sure how the tag didn't end up on the issue when we discussed it last week).

xjm’s picture

Oh, or does #2095195: Remove deprecated field_attach_form_*() already remove all those functions in the third group (or could/should it)?

chx’s picture

Status: Needs review » Reviewed & tested by the community

This is lovely!

webchick’s picture

Title: Remove deprecated procedural functions in Field API » Change notice: Remove deprecated procedural functions in Field API
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Ok. I think that split makes sense. Thanks for the background.

Committed and pushed to 8.x.

We'll need a change notice if we don't have one already.

yched’s picture

Thanks folks :-) Will look into the change records asap.

@xjm #13 / #15
#2095195: Remove deprecated field_attach_form_*() currently removes core calls but keeps the existing functions, as per #9.
Also, current in-progress patch only deals with field_attach_form_*() and is still debated, so if things don't progress fast enough I'm considering maybe splitting a separate issue for field_attach_*_view()...

xjm’s picture

Filed #2167167: Remove field_info_*() and suggested a re-scope for #2095195: Remove deprecated field_attach_form_*() (since it was already removing all usage, it's completely within scope and preferable to remove the functions themselves there as well, since we've committed to removing them before beta).

yched’s picture

re: change notice
So far we updated the existing change notices for the respective topics with further changes.
I.e. :
- field_info_field_types() / field_info_field_settings() / field_info_instance_settings
would go in Field types are now plugins
- field_info_widget_types() / field_info_widget_settings()
would go in Field widgets are now plugins
- field_info_formatter_types() / field_info_formatter_settings()
would go in Field formatters are now plugins
- field_read_*()
would go in Field and field instance are now configuration entities
...

Do we go for this, or do you want separate change records ?

xjm’s picture

Yep, I think updating the existing change records makes sense. Thanks!

yched’s picture

Title: Change notice: Remove deprecated procedural functions in Field API » Remove deprecated procedural functions in Field API
Status: Active » Fixed
Issue tags: -Needs change record

Updated the change notices:

field_info_field_types() / field_info_field_settings() / field_info_instance_settings():
https://drupal.org/node/2064123/revisions/view/6814725/6817957

field_info_widget_types() / field_info_widget_settings()
https://drupal.org/node/1796000/revisions/view/2889377/6817965

field_info_formatter_types() / field_info_formatter_settings()
https://drupal.org/node/1805846/revisions/view/6814731/6817973

field_read_*()
https://drupal.org/node/2012896/revisions/view/2864639/6817981

field_get_items()
https://drupal.org/node/2055615/revisions/view/2786839/6818019

field_get_default_value()
https://drupal.org/node/2070839/revisions/view/2811775/6818009

field_access()
was already covered by https://drupal.org/node/2101719

yched’s picture

Side note:

- Updated $entity_type argument removed from Field API functions and hooks dealing with a single $entity and removed the functions / hooks that have disappeared since then...
https://drupal.org/node/1882428/revisions/view/2549066/6818073

- field_get_items() returns an empty array, rather than false, when no items exist is now irrelevant and should be removed, but it seems I don't have the proper permissions to do this.

xjm’s picture

I deleted the second one. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.