Bug #28980
closed
t3lib_http_Request must not extend PEAR HTTP_Request2, but should encapsulate it instead
0%
Description
Currently t3lib_http_Request extends PEAR's HTTP_Request2 class. By doing so all methods of HTTP_Request2 are exposed as public API. If at some point, for what ever reason we decide HTTP_Request2 to replace it, this will be a problem as we would have to replace the whole HTTP_Request2 public API like it is.
To avoid this and make replacing of the 3rd party library easier we should encapsulate HTTP_Request2 instead of extending it. We then only expose the methods we really need.
Summarizing reasons to encapsulate instead of extending:- we may not want or need all the methods a 3rd party lib offers
- when updating the 3rd party lib, methods may be added or removed without us noticing, again including unwanted changes or methods
- naming of methods may not fit our naming conventions
- abstraction and decoupling to make replacement easier
- easier unit tests as only exposed APIs need testing
- lib is not actively developed anymore
- other libraries are performing better
- other libraries provide better APIs
- other libraries provide better features
- other libraries have fewer bugs
Updated by Philipp Gampe over 12 years ago
So what methods do you want to make public? And what do you want to hide?
Updated by Ingo Renner over 12 years ago
Philipp Gampe wrote:
So what methods do you want to make public? And what do you want to hide?
That's what I was about to ask :)
Even if we simply encapsulate all the methods, that's still better than the current situation. I have to admit though, I haven't had a deeper look at HTTP_Request2. I just stumbled over this issue as I was looking for methods t3lib_http_Request offers and assuming it would be that way already (encapsulated).
Do you have a "must have" list maybe?
Updated by Philipp Gampe over 12 years ago
Do you have a "must have" list maybe?
Pretty much all methods.
It is already rather encapsulated. The real action is performed by adapters and of course by the observers if we attach any.
So what we could do is to add all method with phpdoc and a call to parent::method(). I am not sure if this make much sense, but I guess this is what you want to have.
We need all getters and setters and the send method. I fear there are not much more ;)
If we want to encapsulate, we would also need to encapsulate the response, which is HTTP_Request2_Response currently.
I am preparing some documentation on http://wiki.typo3.org/T3lib_http_Request
This is currently more a copy and past from the manual at PEAR
I will need to rewrite it to some smaller, more TYPO3 specific examples, but the current (very) big example should give you a start about what you can do and what you will need.
To me the class t3lib_http_Request is just a wrapper to set some TYPO3 config defaults. And that is the current state. The question is more what you to how and how you see t3lib_http_Reques: Is it just a config wrapper, a complete wrapper hiding the API or should it enable us to quickly change the underlaying framework.
Updated by Philipp Gampe over 12 years ago
If course I could give you a must have list from linkvalidator, but this wont bring as much further.
https://review.typo3.org/#change,4261
Updated by Xavier Perseguers over 12 years ago
- Status changed from New to Rejected
- Priority changed from Must have to Won't have this time
After thinking about this request. I must say that even if I understand the idea behind this change and they make sense in theory, this would be much too hassle to do that just "in case" we want to change the library.
I find the approach too academical and I prefer the following pragmatic approach:
- There is no other library that seems, currently, to fit our needs
- Whenever it turns out that a new one would be a good candidate for a replacement, we will have to proper test it anyway
- Once we decide to change our underlying library, or in parallel with the previous bullet point, we easily can create the "missing" wrapper methods to either "officialize" the API TYPO3 is providing (API does not change, underlying library changes) or just create the wrapper and mark it as deprecated to get rid of it after 2 versions.
This way, we have the best developer experience and we don't clutter the TYPO3 API with most probably dumb methods that may "never" be changed. In addition, this saves us an additional method call which is never a bad idea to do :-)