Notes on converting apps from Horde 5 to Horde 6

These are by far not complete.

Conversion Status

Library strategies

Parallel

Parallel strategy means double maint. burden but high freedom for due improvements and necessary BC breaks.

Example: horde/test

• Keep unnamespaced PSR-0 code as-is or only make changes required by composer.
• Add PSR-4 PHP 7.4+ code in src/ - it may contain BC breaks and structural changes.
• develop new code quality measures only against the src/ version (phpstan, member/parameter/return type hinting, strict types)
• Convert unit tests for phpunit 9.x, use PSR-4 for unit tests, test against the namespaced version
• src/ Code must be php 7.4 compatible and php 8 compatible.

Wrapper

Wrapper strategy allows both old and new calling code to leverage the same slightly modernized code base. However, BC breaks must be prevented.

Example: horde/components, horde/injector

• Move code to src/ and namespace it
• Add wrappers or child classes to lib so consuming code still works
• Be conservative about return type hinting, parameter type hinting and other potentially BC breaking measures
• Convert unit tests for phpunit 9.x, use PSR-4 for unit tests, test against the namespaced version - maybe keep some duplicate tests against the wrapper code to prove correctness.
• All Code must be php 7.4 compatible and php 8 compatible.

Modern

Newly developed code should only focus on the composer use case

Example: horde/horde-installer-plugin, horde/http_server

• Follow PSR-4, PSR-12, PHP 7.4+, ignore PEARisms unless they belong to the problem domain.
• PHPStan level 4 and higher.
• member/parameter/return type hinting, strict types
• Avoid mixed parameter and return type designs.
• Avoid array constructors if it makes sense. (When switching to PHP 8, we will get named arguments)
• src/ Code must be php 7.4 compatible and php 8 compatible.

Keep

Libraries which are essentially deprecated but kept around for H6

Example: horde/controller

• Only necessary changes to avoid breaking in modern PHP
• unit tests should be PHPUnit 9 ready

General

• Unnamespaced code mostly follows Horde 5 Coding standards and very little conversion requirements
• With some necessary exceptions, Horde 6 should be able to cooperate with Horde 5-ish apps and libraries with very little modification for composer compat
• Unconditional require/require_once/include to paths outside the library's own lib or src dir need to check for the require'd class first (likely breaks in H6 via composer).

[Unnamespaced code still needs Horde_Autoloader's slightly different approach than composer's autoloader]

• Consider: Horde 5 Framework lived out 2012-2021, I would like Horde 6 to be intentionally shortlived, deprecating more stuff over time.
• Allow a very smooth transition path, avoid big bang changes. [STILL TRUE]

Exceptions to the rule

• Most of the upgrade requirements are only targeted at namespaced code

Mandatory

• Horde:: is deprecated [NO REAL REPLACEMENT YET]
• [rla thinks] we should adopt PSR-2, PSR-12 wholesale with an optional opt-out for protected/private underscoring [RLA DOES THIS ALREADY]
• Composer based setup must work with all libraries we intend to keep [SOLVED]
• [rla thinks] Deprecate Horde_Forms usage. Keep it where it is until we can refactor, but make it clear this is a legacy concept
  • Looked into it. There's a few parts to easily refactor when converting to namespaced, with some BC breaks but very schematic remedy. This will reduce warnings and improve compat with newer PHPs. It will really make Form more attractive. Horde_Forms is a major deal breaker for PHP 8.
  • Update Horde_Core to support both namespaced apps non-namespaced apps and mixed. [ONGOING]
• Update Horde_Test to support namespaced unit tests (PHPUnit 9 spews warnings for unit test classnames different than file name, so there is little use in BC here) [DONE]
• Unit Tests must support the current stable phpunit [DONE]
• Check all pear package dependencies if they are also available via packagist or a proper composer channel rather than pear [MOSTLY DONE]
  • Update skeleton to use
    • horde/http_server,
    • Namespacing,
    • Type Hints where appropriate and
    • Model Interface and Rdo implementation rather than driver architecture [NOT YET DONE]

Optional / Best Practice

• Try to drop usage of pear libraries if we only use little portions of their api
  • Consider if there is a more modern alternative available on packagist
  • Consider if it is easy to
• Unbundle bundled code if you can, use composer dependencies [UNBUNDLED phpunit, sabredav]
• Maintain a branch or fork of Skeleton to showcase Horde Ajax Framework and an SPA frontend
• All URL generation should be moved to Horde_Routes_UrlWriter driven by config/routes.php [NOT DONE]
• A more copy/paste ready boilerplate for ClassLevelDocBlock?, FileLevelDocBlock?, MethodLevelDocBlock?
  • Consider: Maybe some custom rules for php-cs-fixer

Exceptions To The Rule

• ?

Namespaces and dir layout

Mandatory

• The home namespace for a library is $Vendor\$Name\, e.g. Horde\View
• The home namespace for a subtype library is $Vendor\$Parent\$Name e.g. Horde\Xml\Element rather than Horde\Xml_Element
  • The Home Namespace for an app in $Horde\$Name, e.g. Horde\Turba
    • TODO: Should we allow third party apps to use their proper vendor if the composer package is type horde-app? We could have an optional registry key (B1 clause)
  • The Horde Base app is Horde\Base
    • DISCUSS: Any arguments for calling it Horde\Horde? Should we rename/alias the base app horde/base altogether?
      • Base is an even worse name than "horde". Between Base and Core, things are often mixed or simply on the wrong side.
  • If a library has an interface or base class Horde_$Name, promote it to either:

- Horde\$Name\$Name
- Horde\$Name\$NameInterface?
- Horde\$Name\Base
- Horde\$Name\Constants
- In case of base classes, re-think if we are not better off with an interface and some trait

• Horde:: is Horde\Core\Horde:: though I think we should re-think using it at all
• Un-namespaced controllers go to app\controllers\ dir, namespaced controllers just go anywhere under src/, i.e. src/Middleware, src/Handler, src/Controller with no magic attached.
• Un-namespaced library code goes to lib/
• New or converted namespaced library code goes to src/.

Optional

• Prefer Use statements over prepending unnamespaced code with a backslash \ unless it is single-use and would require "use as" to avoid clashes.

Exceptions to the rule

• Code which was already namespaced before Horde 6 may be left in lib/ if other unnamespaced code depends on it.
  • In this case, don't forget to tweak autoloading rules in .horde.yml and adjust any github actions workflows

PHPDoc / Type Hints / Type Declarations

Remember Liskov:

• Interface/Base class parameters should be as specific as possible. Derived/Implementing classes can only accept the same or less specific definitions
• Interface/Base class return type parameters should be as unspecific as makes sense. Derived/Implementing classes can announce to return the same or more specific definitions.

Mandatory

• PHPDoc all parameters and return values, regardless if you have typehinted them.
• Prefer A|B|null over "mixed"

Optional

• Parameter Type hints for string, int, boolean SHOULD be added to the namespaced interfaces where possible
• Parameter Type hints for classes SHOULD be added to be added to the namespaced interfaces where possible.
• Use Nullable (?) for optional parameters
• Prefer iterable over array if you can to allow later upgrades from arrays to container objects, generators, iterators etc
• Where Type Hints cannot express exactly what a parameter accepts, typehint for "object" or leave typehint altogether. In any case, make the PHPDoc more specific and wordy

Exceptions to the rule

• ?

GLOBALS usage

Whitelist

• For $injector:
  • Horde\Core\Registry
  • For $_GET, $_POST, $_REQUEST
  • Horde\Controller * Horde\Routes * Horde\Core * Horde\Rpc * Horde\Base * Horde\Cli
• for $config:
  • Horde\Base * Horde\Core\Registry * Horde\Core\Config
• for $page_output:
  • ?
  • for $__autoload

Mandatory

• Namespaced Classes which are not in the whitelist must get the listed globals from constructor or method parameter

Optional

• Factories SHOULD use constructor/setter parameters rather than globals if possible.

Exceptions to the rule

• ?

DI / Injector and Constructors

As Horde has evolved over the years, we have different types of classes, sometimes even mixed.

• Classes which implement create-on-the-fly objects with no/few dependencies or are mostly static, e.g. Horde_Url, Horde_Date, Horde_String
• Classes which have one or few instances to cover most or all use cases, e.g. the DB Driver, the registry. They will typically be provided by the injector directly or via a factory without explicit parameters
• Messy stuff which cannot decide if it is a factory or a base class or wants composition, e.g. Horde_Form, Horde_Rpc
• Classes which are intended as Per-Entity objects

Mandatory

• Constructors SHOULD depend on interfaces or base classes, not on the (single) implementation.
• HordeBase? or App configuration SHOULD be depended on as *TODO* object, not as array and explicitly not via global state
• Apps must not care for configuration of other apps, only theirs or HordeBase?
  • Namespaced app-internal objects which implement an interface should be registered with the injector using the class combined with ::class, e.g.

<?php

       // Use a factory to get an implementation
       // In case the factory cannot be autowired itself, you need to register how to get it first.
       $injector->bindFactory(Horde\Kronolith\Calendar\Resource::class, Horde\Kronolith\Calendar\Factory::class, 'create') 
       // In case you want a specific implementation and it can be autowired, just bind it.
       $injector->bindImplementation(Horde\Kronolith\Renderer::class, Horde\Kronolith\Renderer\Default::class); 
?>

• Note there are no strings, this is the class itself - ::class returns the fully qualified strings
• If you have imported the class with use, you can just use the imported name:

<?php

       use Horde\Kronolith\Calendar\Resource;
       use Horde\Kronolith\Calendar\Factory as CalendarFactory;
       use Horde\Kronolith\Renderer;
       use Horde\Kronolith\Renderer\Default as RendererDefault;
       // Use a factory to get an implementation
       // In case the factory cannot be autowired itself, you need to register how to get it first.
       $injector->bindFactory(Resource::class, CalendarFactory::class, 'create') 
       // In case you want a specific implementation and it can be autowired, just bind it.
       $injector->bindImplementation(Renderer::class, RendererDefault::class); 
?>

• Registering an implementation by an arbitrary string is still legal, we even might need it for bc compat with some designs.

<?php

       $injector->bindImplementation('Forms', '\Horde_Forms_Base'); 
?>

Optional

• Constructors SHOULD better depend on interfaces rather than base classes but SHOULD NOT depend on the (single) implementation.
• Try to refactor constructors to only require interfaces which can be provided by the injector (directly or via a factory)
• If a class used to have a very flexible constructor allowing multiple calling convention, try to distill one calling convention and create either static creator methods or an external factory/builder class
• Internal creation of objects from other packages should be left to trivial cases (e.g. Horde_Date). Otherwise it should happen by injecting some instance provider.
• Use sub-injectors to ensure we do not spill application specific bindings into other apps.

Exceptions to the rule

• Dependencies may be created internally with some hardcoded setup in case of convenience/fallback scenarios. We should not overuse this as it encourages relying on it.

reqire/require_once, Horde\Autoloader and Composer Autoloader

Required

• include, require and require_once should only ever happen to setup autoloading or as part of the autoloader or related to non-php code (templates, data)
• make all explicit require/require_once check if the required class is already available for loading.
  • At least for now, we should not drop Horde\Autoloader
• Horde\Autoloader must always act AFTER the composer autoloader, not first

Optional

• Detect composer autoloader and provide a backend which translates Horde\Autoloader runtime API to composer API

UNCLEAR

• Do we still want to support git-tools setups?
  • I have implemented some basic git handling in components now.

package specific notes

horde\imp

• Find replacement for optional dependency File_ASN1 from phpseclib

Horde\Injector

Optional

• Implement PSR-11 but do not deprecate getInstance(), setInstance() [PR exists]

Horde\Date

Optional

* Horde\Date use cases should generally be checked for timezone mismatches e.g. when converting UTC strings from database
* There is some headache potential with Horde\Db implicitly using Horde\Date without timezone information when SELECTing datetime values.
* This is especially annoying with Horde\Rdo
* Horde\Date constructor should always require an explicit timezone and current time
* Provide convenience static methods for "fromThisFormat()", "fromThatFormat()" to keep that complexity out of the value object

Horde\Core

Mandatory:

• Provide an alternative for Horde_PageOutput which only returns strings rather than doing output (needed, otherwise controllers are littered with ob_start())
• Provide a type to wrap horde config array for DI [DONE]
• A lot of class names must be fixed for namespacing

Optional:

• Registry (probably rather 6.1) Provide a more robust inter-app API but don't break the current inter-app API for now
  • Allow passing objects as long as they implement sleep/wakeup so RPC usecases don't break
  • Allow implementing specific APIs and methods in Horde\$App\Api\$Api but still support $App_Api class for non-namespaced
  • Move vfs/webdav implementations to Horde\$App\Api\Vfs

Horde\Rpc

Mandatory:

• Fix the signature missmatch errors [NOT DONE]

Optional:
* Default to JsonRpc? rather than XmlRpc?
* Refactor to controller framework

Library Upgrade strategy (pre release)

Apart from mandatory changes, we should not currently port all horde to the new standard quickly.

Mandatory

• Existing close-to-core libraries should provide some backward compat either by keeping the code in lib/ mostly unchanged or by having lib/ be some wrapper for src/
• Once H6 has been released, H6.x libraries need to keep whatever legacy support they had until H7

Optional

• New libraries should only be implemented according to current best practices
• Existing fringe libraries may be completely converted to namespaces, dropping unnamespaced versions altogether (pre H6 release)

Exception to the rule

• Breaking apps which have not officially been released as Horde 5 is OK, we may fix them later. Still, it's nice to be nice if you can.

App Upgrade Strategy (pre release)

Required

• Apps may change their internal composition in minor releases (middle digit)
• Apps may mix and match src/ and lib/ but may only ever have ONE implementation of a class either src/ or lib/. (internal consistency)

Optional

• Apps should move from unnamespaced library usage to namespaced library usage whenever feasible even if there is a compat layer.

Developer's wishlist

• JSON version of the topbar
• Have more control over the prefs menu (passwd app related prefs should merge with global/horde prefs related to passwords etc)
• Have a version of PageOutput? that returns a string or a stream
• Make the Themes & JS output useful for custom view types without default prototype/jquery stuff (React UI or similar use cases)
• Assist with user-customized translations
• Default way to expose translations to js world