[dev] components app converted to PSR-4 (mostly) - want feedback

Mon Nov 9 14:44:09 UTC 2020

Zitat von Ralf Lang <lang at b1-systems.de>:

> Hi Jan,
>
> Am 09.11.2020 um 09:29 schrieb Jan Schneider:
>> Hi I didn't find enough time to review the complete PR in detail, but
>> here comes a bit of initial feedback.
>>
> I appreciate it very much nonetheless.
>
>>
>>> Conversion strategy:
>>>
>>>   * PSR-4 code lives in src/
>>
>> I thought about whether we should move classes to src/ too, because
>> that's what most projects do these days. OTOH there is no requirement
>> to do so, and we could as well stick with our traditional lib/, just
>> with the PSR-4 layout. I don't have a strong preference though.
>>
>   * Only few parameter type hints were added or fixed. This should be
>>>     the focus of a second, separate refactoring pass
>>
>> Agreed.
>>
>>>   * NULL changed to void
>>>   * Some shim code for PSR-0 class names expected by by git-tools is
>>>     provided in lib/
>>
>> See above, this is elegant for the migration, but technically not
>> required, right?
>
> Neither having the namespaced code in src/ nor having un-namespaced
> shims in lib/ is technically required.
> I like to have the shims around for now as a compat measure but I don't
> want the in the actual class files.
> Also, in libraries I use the same pattern.
>
> I have not yet upgraded horde/horde and horde/core to the point where
> all integration points can be namespaced. This is not really an issue
> for the components app as it does not rely on registry, rpc, dav,
> activesync, ...
>
>
>>>   * Injector registers both fully qualified name
>>>     Horde\Components\Foo\Bar and shorthand Foo\Bar. The first is for
>>>     autowiring, the second is for convenient use in
>>> injector->getInstance
>>
>> I'd rather go with the FQCN only, we should use
>> Horde\Components\Foo\Bar::class anyway instead of a string, so that
>> you can use any alias or import that you picked in the consumer code.
>
> I'm not quite sure I get this comment right.
>
> To the injector, the identifier is an arbitrary string representing a
> set dependency (Implementation or Closure/Factory), with a fallback to
> autowiring/introspection if nothing has been set and if it is possible
> at all.
> The PSR-11 spec is quite similar:
>
> "An entry identifier is any PHP-legal string of at least one character
> that uniquely identifies an item within a container. An entry identifier
> is an opaque string, so callers SHOULD NOT assume that the structure of
> the string carries any semantic meaning."
>
> As a convention, always using the fully qualified name of the requested
> interface or base class makes sense as it will always work and consumers
> do not need to guess/look up if there are any shorthands. So much I
> agree. It makes for long names, but this is OK with me.
> At the moment, requiring "Horde/Components/Foo" just works as expected.
> I think for using a string Horde/Components/Foo::class we'd need to
> patch the autoloader. For using the actual class as identifier as in
> $injecto->getInstance(Horde\Components\Foo::class) , I've never tried that.

It's not a string, it's a language construct:

Horde\Components\Foo::class === '\\Horde\\Components\\Foo'

This helps IDEs, plus you can use imports, which makes it eventually shorter:

use Horde\Components\Foo;
$injector->get(Foo::class);

>   * I prefer explicitly calling un-namespaced classes as \Horde_Class
>>>     rather than "use"ing them to import them into the namespace.
>>
>> I'm torn on this one, because I didn't see compelling reasons for either.
>>
> If anybody says one of these ways is right, I will go with it. I just
> needed to go anywhere for the  first draft.
>
>>>   * Where the actual class name is ambiguous or misleading, I have used
>>>     the convention to just prepend the last part of the namespace, i.e.
>>>     use Horde\Components\Pear\Environment as PearEnvironment. I did this
>>>     without any thought if reversing the order might be more natural.
>>>     Let's discuss and decide. Example:
>>>
>>> use Horde\Components\Component\Factory as ComponentFactory;
>>
>> Make sense.
>>
>>> use Horde\Components\Config;
>>> use Horde\Components\Config\Bootstrap as ConfigBootstrap;
>>> use Horde\Components\Dependencies;
>>> use Horde\Components\Output;
>>> use Horde\Components\Release\Tasks as ReleaseTasks;
>>> use Horde\Components\Runner\Change as RunnerChange;
>>> use Horde\Components\Runner\CiDistribute as RunnerDistribute;
>>> use Horde\Components\Runner\CiPrebuild as RunnerCiPrebuild;
>>> use Horde\Components\Runner\CiSetup as RunnerCiSetup;
>>> use Horde\Components\Runner\Composer as RunnerComposer;
>>> use Horde\Components\Runner\Dependencies as RunnerDependencies;
>>> use Horde\Components\Runner\FetchDocs as RunnerFetchDocs;
>>> use Horde\Components\Runner\Installer as RunnerInstaller;
>>> use Horde\Components\Runner\Qc as RunnerQc;
>>> use Horde\Components\Runner\Release as RunnerRelease;
>>
>> ReleaseRunner would make more sense.
>>
>>> use Horde\Components\Runner\Snapshot as RunnerSnapshot;
>>> use Horde\Components\Runner\Update as RunnerUpdate;
>>> use Horde\Components\Runner\WebDocs as RunnerWebDocs;
>>
>> Essence: we should use aliases that make most sense from a linguistic
>> POV, but if you create these aliases automatically, sticking to a
>> simple, technical rule is fine too in a first step.
>>
> Agreed.
>>> In most cases, I kept the directory structure and adopted it for
>>> namespace and class names. In few cases, I opted put for obvious
>>> reasons. No Trait should be named ...\Trait and the Components class is
>>> Horde\Components\Components as PSR asks to have at least two levels
>>> Vendor\PackageName\
>>>
>>>
>>>
>>>
>>>
>>>
>>> See https://github.com/maintaina-com/components/tree/maintaina-bare-psr4
>>> for a branch without updated metadata, easy for upstreaming, low chance
>>> of conflict
>>>
>>> See https://github.com/maintaina-com/components/tree/psr4-composerfixed
>>> for a branch with updated metadata, composer.json, changelog,
>>> package.xml, high chance of merge conflicts as soon as anybody does
>>> anything upstream. Use the latter version to test against the
>>> horde-deployment/composer based installation.
>>
>>
>>
> --
> Ralf Lang
> Linux Consultant / Developer
> Tel.: +49-170-6381563
> Mail: lang at b1-systems.de
> B1 Systems GmbH
> Osterfeldstraße 7 / 85088 Vohburg / http://www.b1-systems.de
> GF: Ralph Dehner / Unternehmenssitz: Vohburg / AG: Ingolstadt,HRB 3537

-- 
Jan Schneider
The Horde Project
https://www.horde.org/