6.0.0-git
2020-11-30
Last Modified 2020-11-12 by Ralf Lang (B1 Systems GmbH)

Notes on converting apps from Horde 5 to Horde 6

These are by far not complete.

General

  • Unnamespaced code mostly follows Horde 5 Coding standards and very little conversion requirements
  • Most of the upgrade requirements are only targeted at namespaced code
  • 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
  • Deprecate PSR-0 autoloading but don't break it intentionally.
  • 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.

Exceptions to the rule

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

Mandatory

  • Horde:: is deprecated
  • [rla thinks] we should adopt PSR-2, PSR-12 wholesale with an optional opt-out for protected/private underscoring
  • Composer based setup must work with all libraries we intend to keep
  • [rla thinks] Deprecate Horde_Forms usage. Keep it where it is until we can refactor, but make it clear this is a legacy concept
  • Update Horde_Core to support both namespaced apps non-namespaced apps and mixed.
  • Update Horde_Test to support both namespaced and unnamespaced unit tests
  • Unit Tests must support the current stable phpunit
  • Check all pear package dependencies if they are also available via packagist or a proper composer channel rather than pear
    • Update skeleton to use
      • Horde Controllers,
      • Namespacing,
      • Type Hints where appropriate and
      • Model Interface and Rdo implementation rather than driver architecture

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
  • 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
  • A more copy/paste ready boilerplate for ClassLevelDocBlock, FileLevelDocBlock, MethodLevelDocBlock

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?
    • If a library has an interface or base class Horde_$Name, promote it to either:

- Horde\$Name\$Name
- Horde\$Name\Interface
- 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\, namespaced controllers go to src/Controller
  • Un-namespaced library code goes to lib/
  • Namespaced library code goes to src/.

Optional

  • Unnamespaced code should be preceded with backslash \ to ensure we do not run into issues with namespaced derived classes or use cases

Exceptions to the rule

  • Code which was already namespaced before Horde 6 may be left in lib/ if other unnamespaced code depends on it

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 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.

       // 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:

       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.

       $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?

package specific notes

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
  • 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

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.