Skip to content

Deprecation helper#19

Draft
amulet1 wants to merge 2 commits into
horde:FRAMEWORK_6_0from
horde6:deprecation_helper
Draft

Deprecation helper#19
amulet1 wants to merge 2 commits into
horde:FRAMEWORK_6_0from
horde6:deprecation_helper

Conversation

@amulet1
Copy link
Copy Markdown
Member

@amulet1 amulet1 commented Mar 16, 2026

1. Add DeprecationHelper utility class

  • Emit deprecation notices pointing to the actual call site in user code
  • Transparent redirection of deprecated classes to their replacements via class_alias()
  • Resolve call site by walking the backtrace past autoloader frames

The helper can be used to redirect to another class by replacing the old class file with these 2 lines:

<?php
Horde\Util\DeprecationHelper::redirectClass('Horde_Array', 'Horde\Util\ArrayUtils');

Of course it would be nicer to have a single root helper class, so we could do this:

DeprecationHelper::redirectClass('OldClass', 'NewClass');

or

DeprecationHelper::redirectClass('OldClass', NewClass::class);

In addition to the actual redirecton it also calls trigger_error (or, optionally, error_log) to output deprecation notice with exact location of the deprecated code. It relies on autoloader to load old class, so only the first occurrence gets reported (overhead is small).

The debug mode output example in case of trigger_error:
[horde] PHP ERROR: The Horde_Array class is deprecated. Switch to Horde\Util\ArrayUtils (.../vendor/horde/util/lib/Horde/Variables.php:364)

And in case of error_log (if 3rd parameter is false):
DEPRECATED: The Horde_Array class is deprecated. Switch to Horde\\Util\\ArrayUtils (.../vendor/horde/util/lib/Horde/Variables.php:364)

TODO: It might be a good idea to adjust Horde's error handler to suppress backtrace output for E_USER_DEPRECATED case (the helper already reports exact location).

2. Deprecate Horde_Array class (redirect to PSR-4 implementation)

This provides a working example, but many other classes could be redirected the same way, provided their methods/parameters are 100% compatible.

TODO: Add more helper methods for other scenarios (e.g. emit custom message and pinpoint location of the caller) for deprecated method or method parameters.

amulet1 added 2 commits March 16, 2026 13:44
- Emit deprecation notices pointing to the actual call site in user code
- Allow to redirect deprecated class names to their replacements via class_alias()
- Resolve call site by walking the backtrace past autoloader frames

To be expanded.
@amulet1 amulet1 requested review from TDannhauer and ralflang March 16, 2026 18:11
@ralflang
Copy link
Copy Markdown
Member

At least horde/util has no hard dependencies on anything notable. I will check details later.

Copy link
Copy Markdown
Member

@ralflang ralflang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with putting the helper here if you don't want a dedicated component for it.
I don't agree with log spamming library users for using a migration path we actively offer and still use in 25+ places though.

Do not use this on Horde_Array right now.

@amulet1
Copy link
Copy Markdown
Member Author

amulet1 commented Mar 17, 2026

It feels like a way to for many cases where we have unused cone in src/ (such as Horde\Util\ArrayUtils and many others). If we do not do it for Horde_Array (why?), then what is the criteria?

Let's deprecate redundant code in lib/ where the equivalent code already exist in src/ or else remove code in 'src/' if we are not migrating to it. Anything else (e.g. partial migrations!) would cause future issues.

Let's agree to never change existing methods except for one of these specific cases:

  • method is clearly for internal use only
  • we still fully support existing functionality and parameters (e.g. we are adding new optional parameters).

If anything needs to be changed in incompatible way (even if just method signature), let's do this:

  • implement it as new method (i.e. method with a new name!)
  • announce deprecation of the old method (e.g. in the changelog/docs/mailing lists), mark is deprecated
  • possibly adjust old method to rely on the new method
  • migrate to the new method in all Horde packages
  • (eventually) eliminate old method (assuming 3rd parties, if any, migrated as well)

If we do not do it this way, we will create issues. Note, this obviously does not apply to new classes.

Once again, my main concern is existing duplicated and unused code (such as Horde\Util\ArrayUtils vs Horde_Array). Either we migrate to it or else drop it. Changing the duplicate to deviate from the original code and then trying to migrate to it is bad idea for many reasons mentioned before. The path should be "migrate->change" or "change->migrate", not "break and migrate".

@ralflang
Copy link
Copy Markdown
Member

ArrayUtils isn't unused though.

@amulet1
Copy link
Copy Markdown
Member Author

amulet1 commented Mar 20, 2026

With classmap in composer.json the Deprecationhelper is useless, anyway.

@amulet1
Copy link
Copy Markdown
Member Author

amulet1 commented May 13, 2026

One more attempt to explain why I am in favor of DeprecationHelper and class_alias():

Suppose we have an OldClass and we would like to modernize it (e.g. eliminate old methods in favor of new ones, implement strict types, additional parameters, etc.). To avoid making changes in the legacy OldClass we decide to introduce a modern NewClass — serving the same purpose but with improved functionality.

Now, how do we migrate from OldClass to NewClass?

The approach that is employed now is like this:

  1. Let's begin by switching AppA from OldClass to NewClass.
  2. We discover that AppA actually uses some classes/functions from AppB which only supports OldClass. We are not ready to migrate to NewClass in AppB just yet, as AppB is used by many other apps.
    No problem, let's just change any declarations like OldClass $var to OldClass|NewClass $var, so things continue to work.
  3. Things seem to be more or less okay, should we continue with another app? Oh no, we notice that AppA actually also passes NewClass to AppC, and it breaks things. We are now back to the same compatibility problem described in step 2.
  4. OK, we now have a number of places that use both OldClass and NewClass interchangeably. Let's see if we can eliminate OldClass without breaking anything.
  5. The transition is going somewhat slowly as we discover more issues along the way. We are not in a rush and we also have other things in the pipeline, so it is okay — or is it?
  6. After some time we decide that NewClass was not good enough, and a better implementation is needed.
    We start by making changes in NewClass and realize that it is hard not to cause breakage — because NewClass is already used interchangeably with OldClass, any change to NewClass risks breaking the places relying on that interchangeability. We then introduce NewClassV2 with greatly improved logic and restart at (1) - except we now have places in the code relying on old OldClass or NewClass (or maybe places supporting both of them, but not NewClassV2).

There are obvious problems with the above approach.

  1. We switched AppA to NewClass, and we are now utilizing its new methods in AppA. However, because NewClass is used interchangeably with OldClass in (2), we have to be careful not to cause compatibility issues. In AppB, AppC, ... we are either forced to use only methods/properties that are compatible with both OldClass and NewClass (which defeats the purpose of NewClass!) or else check the type that was passed and use different logic depending on whether we have OldClass or NewClass (this tremendously complicates things because OldClass and NewClass are not compatible!). In both cases we are not getting any closer to moving to NewClass.
  2. Add to this possible serialization issues - unless we specifically take care of it by making OldClass forward compatible (wait, did we not decide not to touch it?) and NewClass backward compatible (this is needed, and it is okay).
  3. We end up with a mixture of old and new code with no clear migration path and a high probability of breakage.
  4. During the transition we still have to maintain two versions of almost everything related to OldClass and NewClass and remember about hard-to-enforce compatibility when using OldClass|NewClass $var.

The DeprecationHelper + class_alias() approach avoids all of the above:

  1. Make sure NewClass fully replicates OldClass methods and parameters (either literally copy the code or make sure we have the same names/parameters for all visible properties/methods). This may require some upfront effort, but it is a one-time cost.
  2. Implement the desired new methods and properties in NewClass. Mark obsolete methods and properties as deprecated (this would help to find them more easily by checking debug logs). Make sure to maintain backward compatibility for now.
  3. Delete OldClass and use DeprecationHelper::redirectClass() in its place — this is essentially a class_alias() pointing to NewClass, with an added deprecation notice to help track down any remaining references to OldClass. Barring edge cases unlikely to apply here, this should not cause any issues.
  4. We can now freely use new methods wherever we want. It is easy to identify and update places still using OldClass and old methods/parameters - as these would have OldClass type (or maybe no type). In the meantime things continue to work.
  5. Once OldClass is no longer used anywhere, we eliminate its redirection to NewClass. In absence of issues we then remove obsolete stuff from NewClass.

The most important part here is that we do not have to maintain compatibility with both old and new classes in multiple places and the migration is straightforward with minimal risk of breakages.

Note that serialization issues are not fully resolved by this approach either — NewClass still needs to handle objects serialized under OldClass. However, at least the forward compatibility concern for OldClass disappears entirely, since OldClass no longer exists as a separate class.

Also note that DeprecationHelper::redirectClass() is not compatible with classmap autoloader type. To solve this we would either need to go back to PSR-0 for older classes or else have a central place (loaded once) with current redirections.

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.

2 participants