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