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

[Ralf Lang]

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.

  * 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