Split files for PSR-4 compliance

[roelofr]

There were many classes combined into single files, which isn't recommended and makes maintaining hard.

For example; Did you know that \JakubOnderka\PhpParallelLint\ArrayIterator was actually located in src/Settings.php. Not where you'd expect it to be.

This PR moves all these classes to their own files, and configures Composer to use a PSR-4 autoloader instead of a classmap.

Closes #25
Closes #26

[jrfnl]

While I agree that this is a step in the right direction, I'd like to suggest taking things a step (or two) further if @grogy agrees.

Suggestions:

1. Implement Rename Jakub's namespace #26 (rename the namespace) at the same time.
2. Use more subdirectories to place classes in logical groups.
   For example (more groupings may be possible, but I haven't looked at everything):
   • Output for the Output interface and all classes extending it.
   • Exception[s] for all Exceptions

Yes, that would be a BC-break and necessitate a major release, but that is needed anyhow when addressing #26, so we may as well try to set it up in a way which will make maintenance more intuitive for the future.

[roelofr]

Yeah, when I was refactoring the code I noticed that my src/ folder was growing at a rapid pace, with Exceptions not being namespaced.

I dabbled about doing that too, but being a first-time contributor to this project, that seemed a bridge too far to cross on the first PR.

[jrfnl]

@grogy Could you please pitch in to say whether you agree with the suggestions above ?

Would be great if we could get this PR moving again to eventually get it merged.

[roelofr]

Okay, the codebase is split up into some folders, and I ran phpstan to make sure all links were correct.

Unit tests still fail locally (something about JsonSerializable not being found, while ext-json is active).

[grogy]

I readed changes commit to commit and it makes sense. I see a problem that I missed that PR and codebase is far away.

For resolve and merge I can rebase and solve it locally and force push it, or you can rebase it onto current master?

[roelofr]

I've rebased the commits, the only big change seemed to have been the GitLabOutput.

A difference since the previous base is here

[roelofr]

I noticed the tests were failing on a missing polyfill, so I've added that back in.

[jrfnl]

I noticed the tests were failing on a missing polyfill, so I've added that back in.

Please load that file via a require_once in the bootstrap instead of letting Composer autoload it. See the dicussion about this here: #51 (comment)

[roelofr]

@jrfnl I tried removing it, but that doesn't work in combination with PHP 5.3, so I re-added it as autoload-dev, which should prevent it from loading in projects requiring this package.

[jrfnl]

I tried removing it, but that doesn't work in combination with PHP 5.3, so I re-added it as autoload-dev, which should prevent it from loading in projects requiring this package.

Using autoload-dev feels wrong as the polyfill is needed for non-dev when the JSON extension is not available. Hmm.... I don't have to time to look into this further at the moment, but I do think we need to find another solution.

[roelofr]

I've removed the commit for now, but as said, it won't work.

The assumption made in the comment mentioned by @jrfnl is, in my opinion, out of scope.

This would prevent potential issues with other dev tools which may use an interface_exists() check on JsonSerializable which would now suddenly start passing for them due to the file being automatically loaded (even when this tool is not being run).

If devtools use the existence on JsonSerializable to check if json is installed, they won't work on php 5.3 to begin with.

Wrapping the polyfill in a version_compare(PHP_VERSION, '5.3', '<=') should therefore suffice, and we can still include the polyfill via composer.

[roelofr]

@jrfnl Did you manage to think about how we're resolving the polyfill issue? I think including it in composer.json is okay, as it's what we're doing in our current latest version... 🤔

[jrfnl]

I've taken the liberty to rebase this PR and add an extra commit changing the namespace prefix to PHP_Parallel_Lint\PhpParallelLint to be in line with the other repos in this organisation.

Will have a look at the PHP 5.3 failure now.

[jrfnl]

(sorry for the long wait, but 🤞🏻 we'll get this merged now soon)

[jrfnl]

I've added an extra commit to sort out the test run on PHP 5.3.

As the polyfilled class will no longer be loaded via the Composer autoload file (as it doesn't have the expected namespace), the file needs to be required in the tests.

When the tests get refactored to PHPUnit, these requires will move to a test bootstrap file instead.

[jrfnl]

@grogy This is ready for merge once PR #92 has been merged and version 1.3.2 has been released.

Might be an idea to (temporarily) create a develop branch and merge this PR to that branch so the changes for the 2.0.0 version are separated out and won't block a potential 1.3.3 release.

[glensc]

PHP_Parallel_Lint as namespace is terrible idea. It likely conflicts with psr-0 that composer has for legacy pear packages. or create some other unknown issues. avoid that. just remove underscores.

[jrfnl]

PHP_Parallel_Lint as namespace is terrible idea. It likely conflicts with psr-0 that composer has for legacy pear packages. or create some other unknown issues. avoid that. just remove underscores.

I hear you, but a) conflicts will only happen if someone would be using the same namespace and b) the PHP_Parallel_Lint prefix has already been implemented in Console Color and Console Highlighter - using a different prefix here would be terribly inconsistent.

[roelofr]

Personally, it stings that the PR I made is now made so ugly with this new namespace change (which I think is really ugly, and a step backwards from no namespaces), but it's been nearly a year and I can't really care anymore.

Good luck with this PR and the project, and thanks for looking into this after a year

[jrfnl]

Personally, it stings that the PR I made is now made so ugly with this new namespace change (which I think is really ugly, and a step backwards from no namespaces), but it's been nearly a year and I can't really care anymore.

Good luck with this PR and the project, and thanks for looking into this after a year

@roelofr I'm sorry you feel this way and I empathize with your frustration about how long the PR has been open. Has happened to me too often as well. The joys of open source....

[jrfnl]

Rebased without changes after the merge of #92 to get past the imaginary merge conflict.

[grogy]

@roelofr thanks for the merge request. I know that long time to merge is frustrating. I am sorry to you.

Today I call with @jrfnl and we discuss about namespaces. We concur that same namespace in whole organization is best way. I am remember that is break change. This change will be released in 2.x version.

Merge request looks good and I does not have now some suggest, so I am merged it. Thanks to all in discussion, @roelofr for original MR and @jrfnl for rebase.