Final class declarations
lkwdwrd opened this issue · 13 comments
Why are so many of the class declarations implemented as final?
I'd love to implement a version of the ProjectFactory as an example, that adds some middleware to the new project definition, but right now I can't. I can implement the interface, sure, but then I'd have to re-implement the entirety of the namespace logic to get a Namespace tree. I can't even wrap the class because the namespace processing is all protected.
Similarly, I'd like to use slightly modified Factory\Function_ and Factory\Method strategies, but would need to copy pasta much of the code because again they are final classes. Being able to extend these classes would be a huge benefit and reduce the amount of code I'd need to write considerably. Perhaps I'm missing something, but they seem constructed with sensible defaults and are itching to be extended for additional implementations. Any reason they are not?
Hello Luke,
That they are final and thus cannot be inherited is by design; although it seems like a large benefit to be able to extend existing classes it is actually detrimental to the design of your application and, more practical, it puts a large burden on us as library developers.
Why, you might ask? In a library you have to keep backwards compatibility not just with the public interfaces, but also with any protected interfaces that can be inherited. If we have every class open (non-final and using protected) that means we can not change the way these classes work since a consumer might rely on a specific piece of behaviour.
In your case you can solve this situation in various ways:
- if it is generic: open a pull request with the fix so that we might include it
- if is it specific, use your own version that indeed copies a lot of the code of the original, or when possible use composition to extend the existing classes. The latter can be done using the Decorator pattern for example.
We have explicitly provided factories so that the code is open for extension (by introducing injection points) and closed to modification (by disabling inheriting these classes) per the Open/Closed principle.
You can find a more extensive write-up on Final here: http://ocramius.github.io/blog/when-to-declare-classes-final/
Does this answer your concerns?
Mostly for now.
Was the choice to disallow middleware in the project out of the project factory also a conscious choice?. I can use composition to accomplish most of what I need, but the project factory provides no way to do that through composition. Is there any chance that will be changed?
At this moment have not had a strong use-case for adding middleware to the ProjectFactory so this was as concious as 'we don't need it yet'; the ProjectFactory is however still undergoing changes when it comes to its API. Based on recent refactorings in the phpDocumentor 3 development branch we are considering changing the ProjectFactory into a builder.
What we can do is take this into account.
@jaapio what do you think about making the namespace building into a piece of middleware that we can inject into the ProjectFactory (and if we decide to implement the ProjectBuilder; to do this as part of the finalization of the builder)?
I don't see any issues the adding middlewares to the ProjectFactory. I think it would be good to extract the namespace building from the factory since it is a complicated part of the factory which was hard to cover with tests. Making it a middleware would make this easier.
But that won't change the interface of the ProjectFactory. Since getNamespaces is part of the ProjectFactory it would be strange to have that not in the factory by default?
@lkwdwrd if you can explain what you need to change I can probably point you in the right direction. Or give you a solution without any changes.
Sorry for the delayed response, between the holidays I haven't had much work time. Unfortunately I've run into additional problems, but I have a better idea now of what/how I'm trying to implement this so I can provide additional specifics.
Basically, I'm looking at what it would take to update https://github.com/WordPress/phpdoc-parser to use the 2.0 version of reflection. Right now it is still pulling in 1.0.0 specifically. Most of the reflection classes were extended at that point, hence the original question.
Now, I've been trying to create a system to gather the additional code meta data using a mixture of composition and decorating. But I'm running into all kinds of problems doing that. I have created a decorator around the ProjectFactory which uses a new set of strategies and then runs everything through a middleware chain. While I would have preferred to use the middleware chain that already exists when creating new file objects, it's fine, plugins can iterate the nodes later at a second middleware chain after the initial parsing and the API access for plugins will be via a post processor.
I wanted to create consistent interface for storing meta in the various reflection objects during this second middleware pass, so I wrapped the reflection objects in a decorator object containing a meta data access layer, then forwarded all other calls back to the original node objects to remain compatible. This works great except the add functions in the reflector objects are type hinted, meaning it throws errors when the decorated object gets sent to the addFunction() method, it doesn't accept it because the object is the wrong type. I'd have to rewrite them to use my own, or at least rewrite large portions of them.
The current system still seems too locked down to use for much beyond the exactly what happens in the original ProjectFactory. Every time I feel like I see a way to work with it I run into another problem where the system only allows the original use-case.
@jaapio - to give you some more specifics about what I'm trying to accomplish:
- After running the parser I need the raw docblock text. I know I can reconstitute the docblocks using the Serializer, but we've toyed with the idea of creating diffs for documentation from the browser. To do this I need to be sure the original text is unaltered from what was in the actual file and the lines it appeared on.
- The parser needs to capture function call information. So if one function calls another it understands that and records it in some kind of 'uses' meta on the reflection object.
- I need to capture 'hooks' which are calls to a handful of functions (
apply_filters,do_action, and a couple others). These have their own docblocks I'll need to capture.
Most of this with the exception of the file-level docblocks could be captured in a post processing pass, but I'd like to work with the reflection system as much as possible rather than just along side it. I'd love a way to standardize meta data making the storage and access layer for it protected and consistent as well as a way to hook into the per-file middleware chain to get and store that meta while the files are being processed.
Thanks!
Basically you want to extend the file object to be able to add more information during the parsing process. Which can be done by adding an new kind of strategy. Adding new strategies can be done by creating a new factory instance without using the createInstance method. But just call the contructor. And provide your own strategies. But due an static list of supported nodes in Reflection/Php/Factory/File::createElements() you are not able to use these strategies and add this meta-data to the files.
I didn't like that switch any way. So we might consider to change that to a more flexible stucture.
A few weeks ago I created a PR to introduce a FileInterface to allow custom FileElement objects. I think we should introduce this interface to allow custom strategies to add new metadata to our file object. Or the FileElement should be extendable. But that will be the only class that can be extended. Since all kind of information that can be stored is part of a file. And can be linked to a class/method/function ect by providing the FQSEN of the element to your meta-class.
Rebuild the ProjectFactory to allow middleware seems to be a good idea. Since this will allow us to extract all logic from the factory. So it can be reused in custom factories.
The raw docblock information is not available in the Docblock class. Since we only store the information we understand. Which should be almost everything, but some parts could be missing. I don't see a good way yet to have this raw infomation. So if you are able to reconstuct it from the current docblock objects that would be nice.
@lkwdwrd Do you think that adding the caller information to the file solves your issue? If so we can start working on the proposed changes.
@jaapio Thanks for your response! I really appreciate you taking the time to work through some of this with me. This issue is starting to have a lot of things going on so I'm going to try and break this out a little bit to talk about each separately. Sorry for the book, trying to be clear.
ProjectFactory Middleware
I agree it would be very useful to have middleware in the project factory. I can get around it easily enough, but would prefer a built in middleware approach.
Element Metadata
It would certainly be possible to use the file object and FQSEN, so if that's the way forward I am all for it. The primary meta I'm looking to capture is usage information. Basically, store which function calls get made internally in the reflected function/method so that I can make relational connections after the fact (e.g. this function uses these functions and is used by these functions). By capturing this we can cross reference by usage in the final documentation.
To me this meta still makes the most sense with the function object itself, not as part of the file, but I could certainly make it work using FQSEN identifiers. I was playing by wrapping the elements like this (please excuse the messy test code) but this is where I hit the type hinting snag. This went deeper than just files. Any time there were nested objects, they're hinted. As an example when a wrapped method got added to a class it wasn't allowed. The hinting means class objects only accept phpDocumentor\Reflection\Php\Method objects, not my wrapped versions.
I think your PR with the file object is good step in the right direction, but curious on your thoughts of more general element meta? The point is not to store random data that doesn't belong, but data that is representative of what's going on inside the element.
Raw Docblocks
This is only a nice to have. It's not critical for what we're trying to accomplish. For elements (functions, classes, methods, etc) I can grab the raw node associated with them and run the getDocComment() method to get the raw text. The only thing this doesn't work for is files.
I played a bit with a custom docblock factory that created a docblock decorated something like this (again, sorry for the messy exploratory code). Type hinting started got me again. All of the reflection element constructors type hint for the phpDocumentor\Reflection\DocBlock so using the wrapped object was a non-starter.
This would only really be useful if/when trying to create documentation patch files using a web interface. The negative part of the diff will need to be exactly the same as the original text, flaws and all. This is a pretty low priority, though, and simply an idea at this point.
Hi Luke,
I had a chat this week with @mvriel about this issue. We will try to refactory this component in steps to something we can use both. Starting with some cleanup actions that will help us to reduce the logic you need to copy when creating new strategies.
Beside that we will do some experiments to remove the switches in the strategies so they can be reused. And we will try to find a way to remove the hard bindings between elements.
FYI: I am spending time on this component to bring it to a new release and intend to review this story in the upcoming period.
I created a Metadata class to make is possible to extend the parser output. A first draft of the docs about this can be found here:
https://github.com/phpDocumentor/Reflection/blob/5.x/docs/meta-data.rst#metadata
I want to have a look at the wordpress docs to see if this implementation fits the needs. Before creating a release.
I did an experiment with phpDocumentor to see if the actual metadata concept could work. And It works for me as expected. I think we have covered the missing use case here to allow people to extend the model when they want to extend it.
The docs are covering this way of using the library. And phpDocumentor/phpDocumentor#3067 can be used as an example implementation.