Convert autoloading to PSR-4 and use composer's autoload

[Girgias]

This requires to bump to PHP 8.0 so that 'Abstract' can be used in a namespace

This might give us better IDE support and better ways to setup testing...

I tried to test this by running it but I can't figure out which command I am meant to enter so I'm hitting issues with trying to render PDF which is expecting the Haru extension to be loaded.... which has been abandoned for ages.

For reference the command I entered is: php render.php -d ../base/.manual.xml -P PHP

[cmb69]

AFAIK, PDF rendering had been retired for practical purposes many years ago. I don't see much point in reviving it.

I'm slightly concerned about a PHP 8.0 requirement. Is our build machine on that version already?

[Girgias]

The only reason why I bumped it to PHP 8 was because of the abstract folder name which can't be used in a namespace before PHP 8, renaming this folder should do the trick.

I'll do that when I get back to this because I was more concerned with getting this running first.

[cmb69]

Are there any objections to merge this?

[kocsismate]

Are there any objections to merge this?

There's none from my side, of course :) I only have just one issue: it would be nice if my other PR could be merged in advance, so that conflicts are avoided

[salathe]

Are there any objections to merge this?

Am I correct in understanding that in order to render docs after this PR, we'd have to run some composer command(s) to generate the autoloader before being able to do php render.php ... as normal? If that's true, then I object.

[Girgias]

Are there any objections to merge this?

Am I correct in understanding that in order to render docs after this PR, we'd have to run some composer command(s) to generate the autoloader before being able to do php render.php ... as normal? If that's true, then I object.

Just composer install which would generate the autoloader and install other dependencies (currently there are none)

[Girgias]

@kocsismate seems #50 inserts a redundant strong tag (and possibly whitespace)

[kocsismate]

I'll fix it asap

[kamil-tekiela]

What is the benefit of using Composer? I think there was a discussion about using Composer in the past but I was not part of it. Can someone summarize why do we need Composer in PHD? If we don't need composer then we can use the built-in autoloader.

But I am generally in favor of moving from PSR-0 to PSR-4

[Girgias]

What is the benefit of using Composer? I think there was a discussion about using Composer in the past but I was not part of it. Can someone summarize why do we need Composer in PHD? If we don't need composer then we can use the built-in autoloader.

But I am generally in favor of moving from PSR-0 to PSR-4

Mainly because we need testing, and the current tests depend on the php-src test runner, which is far from ideal as there is no easy way to pull it in for CI, and the two reasonable options I see is to either make php-src's test runner a separate repo and a project that we can pull in via Composer, or migrate to PHPUnit which we need to pull in via Composer.

Composer is the de facto package manager in PHP and using it seems far from controversial.

Moreover, it could allow us to pull in some packages to improve the codebase, like a DI container.

If the issue of running a script is on the infra side we could add the /vendor/ folder to git tracking.

[kamil-tekiela]

So that was my next question. If we merge this, won't the nightly job on php.net fail due to missing composer install --no-dev -a?

[salathe]

Just composer install which would generate the autoloader and install other dependencies (currently there are none)

Ok. Then I maintain my objection. If this PR gets merged as-is, someone is going to have go through all of the "documentation", which is spread far and wide, to change the "how to use PhD" instructions. Also, someone is going to have to install composer, make sure it keeps up-to-date, change build steps, etc. on at least the two php.net machines that are building the docs.

Back to problems that can be fixed today: rendering the docs from this PR has a few problems...

• A new E_NOTICE, which looks likely due to the autoloader changes.
• Fatal Errors due to a whole bunch of usages of the phpdotnet\phd\v() function, called as v(...) (i.e. no namespace) in other namespaces without first use-ing that function.

For example (showing both of these problems):

$ php render.php --docbook ../doc-base/.manual.xml --package PHP --format bigxhtml
[14:59:40 - E_NOTICE ] <snip>/phd/src/Config.php:89
Undefined index: olderrrep
[14:59:40 - Rendering Style ] Running full build
[14:59:40 - Indexing ] Skipping indexing
[14:59:40 - Rendering Style ] Running full build
PHP Fatal error: Uncaught Error: Call to undefined function phpdotnet\phd\Package\PHP\v() in <snip>/phd/src/Package/PHP/BigXHTML.php:104
Stack trace:
#0 <snip>/phd/src/Format.php(208): phpdotnet\phd\Package\PHP\BigXHTML->update(64, true)
#1 <snip>/phd/render.php(116): phpdotnet\phd\Format->notify(64, true)
#2 {main}
thrown in <snip>/phd/src/Package/PHP/BigXHTML.php on line 104
Fatal error: Uncaught Error: Call to undefined function phpdotnet\phd\Package\PHP\v() in <snip>/phd/src/Package/PHP/BigXHTML.php:104
Stack trace:
#0 <snip>/phd/src/Format.php(208): phpdotnet\phd\Package\PHP\BigXHTML->update(64, true)
#1 <snip>/phd/render.php(116): phpdotnet\phd\Format->notify(64, true)
#2 {main}
thrown in <snip>/phd/src/Package/PHP/BigXHTML.php on line 104

I also tried (because my memory is like a sieve these days) php render.php --list, which is supposed to list all of the packages and associated formats.

$ php render.php --list
[15:22:24 - E_NOTICE ] <snip>/phd/src/Config.php:89
Undefined index: olderrrep
Supported packages:
PHP Fatal error: Uncaught Error: Class 'phpdotnet\phd\Options\Format_Factory' not found in <snip>/phd/src/Options/Handler.php:297
Stack trace:
#0 [internal function]: phpdotnet\phd\Options\Handler->option_list('list', false)
#1 <snip>/phd/src/Options/Parser.php(111): call_user_func(Array, 'list', false)
#2 <snip>/phd/render.php(25): phpdotnet\phd\Options\Parser::getopt()
#3 {main}
thrown in <snip>/phd/src/Options/Handler.php on line 297
Fatal error: Uncaught Error: Class 'phpdotnet\phd\Options\Format_Factory' not found in <snip>/phd/src/Options/Handler.php:297
Stack trace:
#0 [internal function]: phpdotnet\phd\Options\Handler->option_list('list', false)
#1 <snip>/phd/src/Options/Parser.php(111): call_user_func(Array, 'list', false)
#2 <snip>/phd/render.php(25): phpdotnet\phd\Options\Parser::getopt()
#3 {main}
thrown in <snip>/phd/src/Options/Handler.php on line 297

How thoroughly have these changes been exercised / tested? Because basic commands like above should not be failing.

[Girgias]

Just composer install which would generate the autoloader and install other dependencies (currently there are none)

Ok. Then I maintain my objection. If this PR gets merged as-is, someone is going to have go through all of the "documentation", which is spread far and wide, to change the "how to use PhD" instructions. Also, someone is going to have to install composer, make sure it keeps up-to-date, change build steps, etc. on at least the two php.net machines that are building the docs.

So this issue can be by-passed by adding the vendor folder to git and upload it, this would mean for dev we can use it instead of reinventing the wheel and have the infra just pull in all the dependencies from the repo (even if goes against best practices but baby steps).

Back to problems that can be fixed today: rendering the docs from this PR has a few problems...

* A new E_NOTICE, which looks likely due to the autoloader changes.
* Fatal Errors due to a whole bunch of usages of the `phpdotnet\phd\v()` function, called as `v(...)` (i.e. no namespace) in other namespaces without first `use`-ing that function.

For example (showing both of these problems):

$ php render.php --docbook ../doc-base/.manual.xml --package PHP --format bigxhtml
[14:59:40 - E_NOTICE ] <snip>/phd/src/Config.php:89
Undefined index: olderrrep
[14:59:40 - Rendering Style ] Running full build
[14:59:40 - Indexing ] Skipping indexing
[14:59:40 - Rendering Style ] Running full build
PHP Fatal error: Uncaught Error: Call to undefined function phpdotnet\phd\Package\PHP\v() in <snip>/phd/src/Package/PHP/BigXHTML.php:104
Stack trace:
#0 <snip>/phd/src/Format.php(208): phpdotnet\phd\Package\PHP\BigXHTML->update(64, true)
#1 <snip>/phd/render.php(116): phpdotnet\phd\Format->notify(64, true)
#2 {main}
thrown in <snip>/phd/src/Package/PHP/BigXHTML.php on line 104
Fatal error: Uncaught Error: Call to undefined function phpdotnet\phd\Package\PHP\v() in <snip>/phd/src/Package/PHP/BigXHTML.php:104
Stack trace:
#0 <snip>/phd/src/Format.php(208): phpdotnet\phd\Package\PHP\BigXHTML->update(64, true)
#1 <snip>/phd/render.php(116): phpdotnet\phd\Format->notify(64, true)
#2 {main}
thrown in <snip>/phd/src/Package/PHP/BigXHTML.php on line 104

I also tried (because my memory is like a sieve these days) php render.php --list, which is supposed to list all of the packages and associated formats.

$ php render.php --list
[15:22:24 - E_NOTICE ] <snip>/phd/src/Config.php:89
Undefined index: olderrrep
Supported packages:
PHP Fatal error: Uncaught Error: Class 'phpdotnet\phd\Options\Format_Factory' not found in <snip>/phd/src/Options/Handler.php:297
Stack trace:
#0 [internal function]: phpdotnet\phd\Options\Handler->option_list('list', false)
#1 <snip>/phd/src/Options/Parser.php(111): call_user_func(Array, 'list', false)
#2 <snip>/phd/render.php(25): phpdotnet\phd\Options\Parser::getopt()
#3 {main}
thrown in <snip>/phd/src/Options/Handler.php on line 297
Fatal error: Uncaught Error: Class 'phpdotnet\phd\Options\Format_Factory' not found in <snip>/phd/src/Options/Handler.php:297
Stack trace:
#0 [internal function]: phpdotnet\phd\Options\Handler->option_list('list', false)
#1 <snip>/phd/src/Options/Parser.php(111): call_user_func(Array, 'list', false)
#2 <snip>/phd/render.php(25): phpdotnet\phd\Options\Parser::getopt()
#3 {main}
thrown in <snip>/phd/src/Options/Handler.php on line 297

How thoroughly have these changes been exercised / tested? Because basic commands like above should not be failing.

Obviously the testing is lacking because our test suite is lacking, so if you remember a list of commands that should work (or can point me to where to find them) I can manually test them. But thanks for spotting these issues.

[kamil-tekiela]

Are there any objections to merge this?

In the current state, yes. I just pulled the PR to my machine and it lit up in red... so I think I am missing some commits.

I'd much rather see this PR split into smaller chunks. First, move from PSR-0 to PSR-4 and make sure that everything still works. Then if people have no objections to changing the build tools, we can implement Composer autoloading.

I also agree that we should probably remove some of this functionality before we attempt larger refactoring like this, as there's no point in maintaining functionality that is not in use.

[Girgias]

Are there any objections to merge this?

In the current state, yes. I just pulled the PR to my machine and it lit up in red... so I think I am missing some commits.

I'd much rather see this PR split into smaller chunks. First, move from PSR-0 to PSR-4 and make sure that everything still works. Then if people have no objections to changing the build tools, we can implement Composer autoloading.

I'm not sure there is much point in changing the autoloading standard if it's not to use the Composer one, this PR is basically just this move to PSR-4, and the issues causes by function defs is because of the namespace requirements of PSR-4.

I also agree that we should probably remove some of this functionality before we attempt larger refactoring like this, as there's no point in maintaining functionality that is not in use.

This can be done now as a separate PR.

[Girgias]

@salathe I think I've fixed all of the issues and concerns, can I get your opinion on this again?

[cmb69]

Wouldn't having vendor/ pre-installed imply that one must use a certain PHP version?

[kamil-tekiela]

[kocsismate]

Wouldn't having vendor/ pre-installed imply that one must use a certain PHP version?

AFAIK, no, as long as the PHP version requirement is satisfied.

[afilina]

@Girgias How much of this is still relevant today, considering the project is compatible with 8.1+, uses Composer, and seems to be mostly PSR-4.

[Girgias]

Do we use the composer autoload now? If not then this is still somewhat relevant, otherwise can be closed.

[afilina]

@Girgias There's no reference to autoload.php in the codebase.

If not then this is still somewhat relevant

Could you point out exactly which part is relevant in this context? I feel like the merge conflicts are too severe to try and do something with this diff. But we could convert it to an issue with a specific request.

[Girgias]

Superseded by #229