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

Girgias · 2021-07-29T01:36:24Z

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 · 2021-07-29T16:51:34Z

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 · 2021-07-29T17:01:43Z

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.

render.php

cmb69 · 2021-07-30T14:37:23Z

Are there any objections to merge this?

kocsismate · 2021-07-30T14:42:00Z

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 · 2021-07-30T14:52:40Z

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 · 2021-07-30T14:57:12Z

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)

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

Command used: php render.php --docbook ../base/.manual.xml --package PHP --format xhtml

Turns out we need the php-src test-runner....

This allows to drop the version back down to PHP 5.6 (due to 'use function')

Girgias · 2021-07-30T15:08:17Z

tests/php/bug49102-1.phpt


        <div class="classsynopsisinfo">
            <span class="ooclass">
-                <strong class="classname">SplStack</strong>
+                <span class="modifier">class</span> <strong class="classname"><strong class="classname">SplStack</strong></strong>


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

I'll fix it asap

kamil-tekiela · 2021-07-30T15:10:38Z

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 · 2021-07-30T15:15:21Z

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 · 2021-07-30T15:19:08Z

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 · 2021-07-30T15:21:02Z

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 · 2021-07-30T15:32:45Z

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 · 2021-07-30T15:33:41Z

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 · 2021-07-30T15:35:56Z

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 · 2021-07-30T17:03:51Z

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

cmb69 · 2021-07-30T17:06:37Z

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

kamil-tekiela · 2021-07-30T17:07:26Z

kocsismate · 2021-07-30T17:10:00Z

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.

highlight_string() does not throw and even if it did it would throw an Exception which exists

kamil-tekiela reviewed Jul 30, 2021

View reviewed changes

render.php Outdated Show resolved Hide resolved

Girgias added 9 commits July 30, 2021 17:02

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

1c0b3c8

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

Fix rendering

4fe0158

Command used: php render.php --docbook ../base/.manual.xml --package PHP --format xhtml

"Fix" tests

33fc797

Actually fix PHP Package tests

5d940d5

Turns out we need the php-src test-runner....

Move files out of abstract folder

5a9c900

This allows to drop the version back down to PHP 5.6 (due to 'use function')

"Autoload" functions file

233a143

Fix path in util script

e95f437

Update composer.lock file

3858847

Fix test after rebase

e2d8421

Girgias force-pushed the psr-4 branch from 7e83f19 to e2d8421 Compare July 30, 2021 15:05

Girgias commented Jul 30, 2021

View reviewed changes

Girgias added 2 commits July 30, 2021 17:50

Import v() function

c1f0676

Fix render.php Option Handler

fa0cee9

kocsismate mentioned this pull request Jul 30, 2021

Do not render the strong tag unnecessarily #54

Merged

Girgias added 2 commits July 30, 2021 18:30

Fix demo script in API package

ebbf955

Add vendor folder to VCS

ec6e1e5

Girgias added 2 commits July 30, 2021 18:52

Import removeDir() function in PHP/Web

5383027

Move side-effect out of function delcaration file

6869557

Girgias added 3 commits July 30, 2021 19:49

Add missing Config import

c2ba2ea

Remove bogus try-catch block

0c7afa3

highlight_string() does not throw and even if it did it would throw an Exception which exists

Drop vendor folder from git ignore

fbb87c6

kamil-tekiela mentioned this pull request Jul 30, 2021

Remove dead code #43

Closed

Girgias mentioned this pull request Dec 24, 2021

Use PSR-4 rather than PSR-0 #62

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Girgias commented Jul 29, 2021

cmb69 commented Jul 29, 2021

Girgias commented Jul 29, 2021

cmb69 commented Jul 30, 2021

kocsismate commented Jul 30, 2021

salathe commented Jul 30, 2021

Girgias commented Jul 30, 2021

Girgias Jul 30, 2021

kocsismate Jul 30, 2021

kamil-tekiela commented Jul 30, 2021 •

edited

Loading

Girgias commented Jul 30, 2021

kamil-tekiela commented Jul 30, 2021

salathe commented Jul 30, 2021 •

edited

Loading

Girgias commented Jul 30, 2021

kamil-tekiela commented Jul 30, 2021

Girgias commented Jul 30, 2021

Girgias commented Jul 30, 2021

cmb69 commented Jul 30, 2021

kamil-tekiela commented Jul 30, 2021

kocsismate commented Jul 30, 2021

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

Are you sure you want to change the base?

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

Conversation

Girgias commented Jul 29, 2021

cmb69 commented Jul 29, 2021

Girgias commented Jul 29, 2021

cmb69 commented Jul 30, 2021

kocsismate commented Jul 30, 2021

salathe commented Jul 30, 2021

Girgias commented Jul 30, 2021

Girgias Jul 30, 2021

Choose a reason for hiding this comment

kocsismate Jul 30, 2021

Choose a reason for hiding this comment

kamil-tekiela commented Jul 30, 2021 • edited Loading

Girgias commented Jul 30, 2021

kamil-tekiela commented Jul 30, 2021

salathe commented Jul 30, 2021 • edited Loading

Girgias commented Jul 30, 2021

kamil-tekiela commented Jul 30, 2021

Girgias commented Jul 30, 2021

Girgias commented Jul 30, 2021

cmb69 commented Jul 30, 2021

kamil-tekiela commented Jul 30, 2021

kocsismate commented Jul 30, 2021

kamil-tekiela commented Jul 30, 2021 •

edited

Loading

salathe commented Jul 30, 2021 •

edited

Loading