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

[Jan Schneider]

Hi I didn't find enough time to review the complete PR in detail, but  
here comes a bit of initial feedback.

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

> https://github.com/horde/components/pull/4
>
> Hi,
>
> I updated the components app to PSR-4. Mostly I wanted to see if it
> works out as desired and get feedback if anything is creating issues or
> other devs don't like it. This had very limited testing and I expect to
> find more oversights, but I think I have seen most types of conversion
> issues by now. I'd like to merge this once I have fixed Horde_Test and
> components' test suite.
>
> see PR for details.
>
>
>
>   *
>
>     Converted to PSR-4 code
>
>   *
>
>     Dropped conductor from the code base as pear handling does not
>     require it any more
>
>   *
>
>     Added capability to write psr-4 autoloader config into composer.json
>     if horde yml asks for it
>
>   *
>
>     tested actions are qc, changed, composer, update
>
>   *
>
>     Unit tests have not yet been touched and most likely are broken.
>     This needs a PSR-4 aware Horde_Test to make work.
>
> This has only been tested with the composer based setup. Most likely a
> git-tools based setup won't work with it. I am not sure if we find that
> important at this point.
>
> I think this should be improved until the qc command can run component's
> own unit tests before merging.
>
> 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?

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

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

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



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