Codebase is too closed/unflexible

[ostrolucky]

You use final keyword by default for all new classes, without providing way how to extend functionality. That's not how is it supposed to be used.

Biggest sin I see is that you require SourceCollection on 9 places, but SourceCollection is final. SourceLocation is required in SourceCollection, but SourceLocation is final Same thing, Error class.

This is forbidden even by proponents of these declarations Stop making hard dependencies on final classes! If you don't want to make abstractions for these classes, don't make them final.

Second issue is that there is no way to extend functionality of your visitors/extractors. This is ok for small classes, but very disappointing for more complicated ones, like ValidationAnnotation, FormTypeLabel*, FormTypeChoices, etc. If you want to keep them final, please extract parts of their logic into separate classes which can be replaced.

Sincerely, all of the developers who had to copy & paste chunks of your codebase with large complexity levels, having to put it into exceptions for our CI.

[Nyholm]

Thank you for the feedback. Most classes are final by default because it is way easier to remove the final keyword then adding it. I agree that there are some missing interfaces. I would be happy to review a PR to fix that.

Most visitors are relative small and follow the single responsibility principle. I do not see why extending them would make any sense. I would be happy to be proved wrong. IMHO, if you have a custom use case, use a custom visitor.

You could easily change the behavior of the Extractor by just adding a different set of visitors.

[ostrolucky]

Yes, I'm aware why you do this. Not having any responsibility whatsoever for BC. At a cost of blocking any extensibility whatsoever, which is pretty much middle finger for application developers. Good libraries allow extension points. This library consists mostly from set of final classes, which don't have extension points.

Anyway this isn't rant about final classes, but about closed nature of this codebase. You are right that removing this keyword in visitors don't solve anything, because almost all of them consist from single method only. So they wouldn't offer any extensibility anyway. One exception we wanted to utlize was to override ValidationAnnotation::extractFromConstraints.

It's easy to argue visitors follow SRP, because there is no clear line for declaring something to follow SRP. So I don't want to be dragged to this discussion. Fact is, some of the visitors have all logic in single method with more than 30 lines of code and it could be split into more granular focused elements.

if you have a custom use case, use a custom visitor.

That's obvious. Problem is if you want to change one small aspect of current visitor, you have to duplicate whole logic in new visitor.

If you need inspiration how could visitors be made more flexible, just look at reported issues. Almost all of them are reported for visitors. And I believe all of them were solved just by copying the visitor into application and changing that one small thing.

[Nyholm]

How do you propose to make the code base more open and easy to extend? Just removing the final keyword on all classes is not a good enough solution.

[ostrolucky]

Besides inheritance, injection via constructor of course. Easiest is to accept optional closures and use default, inline closures when null has been passed. More heavyweight option is to replace closures with proper, separate classes and interfaces instead..

[Nyholm]

Would you mind sending PRs to achieve this goal?

[ostrolucky]

I don't plan to do this myself, no. This should be ideally done by maintainer. These kind of changes are very hard to make in a way that are accepted by maintainer. I pick my battles. Important first step is to express that you are keen to open up the codebase to more flexibility. I think you did that, so if you don't do it, maybe someone in future who sees this conversation will contribute this.

[Nyholm]

Im happy to invite you to become a maintainer. There are a few of us ATM and you seams to have many good ideas and opinions how to improve. It would be excellent if those ideas could be implemented in the best possible way. I think you are an excellent candidate for making such implementation.

[ostrolucky]

Here are some changes which include major BC breaks I would like to see to be done in next major:

• Rename Extractor to FileExtractorChain
• Create Extractor interface, defining single method with no arguments, accepting SourceCollection
• FileExtractor interface will implement this interface
• Create ExtractorChain class implementing this interface as well

Reason: I'm working on an extractor for enums and wanted to make it as a bridge https://github.com/Elao/PhpEnums/issues/51. During this process, I've found current architecture is tightly coupled to Files. What I want to do though, is to create extractor working with current runtime. This extractor gets all classes via get_declared_classes and checks which ones implement desired interface.

Bonus: After doing this, SourceLocationContainerVisitor can utilize this, which means it will get simplified a lot and be more robust (currently it expects class implements interface directly) - currently it just pretends it's independent from runtime

[Nyholm]

I like what you suggest. Could we do these changes with no or minimal changes to the tests?

[ostrolucky]

I stopped using this library as I moved companies. Might be better to create the PR directly if somebody is interested in this