Deprecate Typed Data's fork of the Rules Context classes

Created on 5 September 2020, almost 4 years ago
Updated 16 March 2023, over 1 year ago

As outlined in the related issues, some Rules Context code was moved to typed_data more than three years ago by commit a98ce54. There is no issue associated with this commit.

Specifically, four classes were copied:

src/Context/ContextDefinition.php
src/Context/ContextDefinitionInterface.php
src/Context/AnnotatedClassDiscovery.php
src/Context/Annotation/ContextDefinition.php

To this day these remain almost-identical copies of the Rules code, with the exception of src/Context/ContextDefinition.php which has diverged for D9 because of #3161582: EntityContextDefinition breaks the context system β†’ .

It is troublesome that the typed_data version of these classes don't seem to be used by typed_data, as evidenced by the related issues - both #3168398: Don't use Doctrine directly β†’ and #3161582: EntityContextDefinition breaks the context system β†’ should have caused typed_data tests to fail. So either we're not using the typed_data classes or our tests need some seriously improved coverage.

Likewise, much of the Context code remains in Rules. If we NEED it here in typed_data, then Rules should be using the typed_data version, not a copy in Rules. Perhaps the Context code should be split of into its own module, but NOT its own composer package as @fago suggested long ago in #2677098: [META] Provide Context related code as composer package β†’ . But certainly we need to examine this and remove the code duplication which can only cause problems in the future as the copies diverge over time.

πŸ“Œ Task
Status

Fixed

Version

1.0

Component

Miscellaneous

Created by

πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • First commit to issue fork.
  • @aaronbauman opened merge request.
  • πŸ‡ΊπŸ‡ΈUnited States AaronBauman Philadelphia

    MR6 adds deprecation notices to the Context classes for 1.x branch.

    See followup issue for 2.0.x πŸ“Œ Remove Context classes Fixed

  • Status changed to Needs review over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States AaronBauman Philadelphia
  • Status changed to Needs work over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

    Thanks. Can you fix up the @deprecated statements so they don't generate those coding standards messages?

  • πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

    Also, don't we need a @trigger_error on the classes?

  • πŸ‡ΊπŸ‡ΈUnited States AaronBauman Philadelphia

    Would you want the trigger_error just inside the class statement? Or within each method?
    I can also look into a deprecated core class and see what they did.

  • πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

    I believe the rules for how to properly deprecate classes are in the coding standards document somewhere - I haven't looked at it recently, but in the changes I made locally (back in December) for testing I put a @trigger_error between the namespace and the first use on all the deprecated classes.

  • Status changed to Needs review over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States AaronBauman Philadelphia

    Updated annotation strings and added trigger_error()s

  • πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

    With this, there should be a blank line between the @deprecated tag and the @see tag.

    Is this the proper way to deprecate Interfaces? I looked it up last year and didn't find any information or examples in core, which is why I never did this myself. Will deprecating an Interface like the above patch actually trigger deprecation notices in code that uses a class implementing that interface? The closest I could find to deprecating an Interface was to deprecate each of the methods individually. Does this need to be done, or can we get away with doing the same thing we do with classes?

    • TR β†’ committed c5d735ca on 8.x-1.x
      Issue #3169307 by AaronBauman: Deprecate Typed Data's fork of the Rules...
  • Status changed to Fixed over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States TR Cascadia

    Committed.

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

Production build 0.69.0 2024