Extension Terror?

When I first saw the posting about ‘bad’ code in OXID over at phpterror I wondered if I could ignore this - but now I’ve realized that this article was published on Planet-PHP and even more - other people start to copy the content of the article.

As I am the guy who introduced the disliked functionality many years ago ( actually years before ZF popped up) I feel the need for a statement to put the things into the right order. Please note that I did work for OXID in the past (years ago) but I do not nowadays.

Arno criticised the way modules == classes are instantiated in OXID eShop. Actually I do believe that exactly this feature is the most coolest in OXID and should be implemented in more OSS.

Let’s start with the “why the fuck did the guy implement this”?

If you do run a website out there you know how important it is to keep it up-to-date with the latest patches due to security reasons. Now imagine - we are in eCommerce area. You deal with payments, credit card data and sensitive information about what people ordered from your shop. Not only Creditcard data is sensitive - imagine it would leak that you ordered the extra-big-boobs doll? You see - especially in this area you need to make absolutly sure that your servers and the software is safe.

Unfortunately you can’t start an eCommerce business out-of-the-box. You need to adopt the software to your needs and processes. Think of payment, ERP, delivery notifications, link to your stock, uploads to price-comparison sites etc. Some of the functionality you might need to develop yourself - for other stuff you might find already existing modules out there.

Therefore you need to change code, and/or install external modules which modify the functionality. In former days you simply edited the source code (as we are talking about OSS) and made manually sure that the modules you install are compatible. Don’t forget - each of them will work with the out-of-the-box shop - but they still should work after you added your changes and - also need to work after you installed some other external modules which might overwrite/change the same classes/functionality which your new module also want to change.

You end up with a highly customized shop, many changes, a lot of work to dig through the sourcecode of the installed modules to make sure that they don’t harm themself and… you lost the possibility to automatically appply patches/releases. Upon each patch or new release you need to manually redo/check your changes.

This sucks. It sucks a lot. And this is exactly why I’ve introduced the criticised module functionality.

So what did I do?

As Arno already copy&pasted the source I won’t repeat it here. But let me explain the main idea behind the concept.

As OXID is fully OOP you can change all the functionality by inheriting your class from any base class you might want to change. You add your changes and - et voila - you still can overwrite the bases classes with new releases/patches and your changes will work. This is called object oriented programming.

Let’s assume you want to change the method “getPrice” in the class “oxarticle”. You simply overwrite oxArticle::getPrice and… it would work if the system would know that your class exists. Therefore you need to register your class and let the OXID Framework know about it so that it automatically will instantiate your class instead of oxArticle each time the object is needed.

So far, so good, so what?

What happens if you install some other module ( e.g. from here) which will change the same method in same class? It would screw your changes and you would end up with manual changes again. To avoid this, the OXID Framework supports “chaining” of inheritance… and the order is set in the config. So you can define that the modules oxArticle implementation is executed before your class or after.

I do believe that by overriding the classes you do get the most flexible option to change everything without loosing the functionality to update the shop whenever you want. For sure this could be solved differently. Events or Hooks would have been a way to go - but this would involve a lot of additional coding == lines of code and therefore introduce new vectors for bugs. And - it is by far not as flexible as overriding the class. Nowadays it could be solved also with Reflection. But this is a bit too “magic” in my opinion. Therefore I still believe that the way I’ve chosen is the best way to solve the problem and - most likely - I would do it again today.

Last but not least - I would like to comment the show me your 94% Unit Test coverage! posting.  I’ve contacted OXID already one year ago about this and I fully agree with Arno here. It sucks to advertise with some test-driven-development features and then keep the tests for yourself. OXID has to change this. Now.

Today I had a good skype discussion about the issue with Arno - I really like his style even though we both do not agree ;) - and the result for me was that  I’ve decided to submit a few proposals to the PHP Conference in Barcelona to get the chance to meet Arno and discuss the whole issue over a bottle of spanish Rioja. Or two. Or…

Comments

27 Responses to “Extension Terror?”

  1. Thomas Koch on August 27th, 2009 3:06 pm

    Shouldn’t you instead use a decent Version Control System instead of implementing patch management inside your application?
    GIT and others may not have been available at the time you created this, but even with SVN people were able track patches. (Take Quilt for example.)

  2. dodger on August 27th, 2009 3:16 pm

    I see I did not make myself clear. We are talking here about a software out-of-the-box which can and will be modified by individual users. The typical shop owner is either a large company or a small vendor and both are not much into patch management.

  3. Vinai Kopp on August 27th, 2009 3:19 pm

    I spend my days developing Magento extensions and must say that having the possibility to chain class overrides is the one thing I envy oxid developers about.
    All the best, Vinai

  4. Community News: OXID eShop - Opinions and Corrections | Webs Developer on August 27th, 2009 4:01 pm

    [...] in this piece from Lars Jankowfsky he tries to set the record straight on a few things: As I am the guy who [...]

  5. Ilia Jerebtsov on August 27th, 2009 4:43 pm

    I’m not sure where I read this, and I can’t seem to Google it offhand right now, but as I understand, one should code for usage, and not inheritance.

    In my experience, expecting people to “overwrite” any behavior in any class is, in my experience, a much more bug-prone approach than to simply provide the required extensibility hooks to be able to execute whatever functions are necessary.

    When you open up your classes for full modification in this manner, you’ve lost control over the system, because you can never guarantee what’s going on with any bit of it. This is analogous to a blacklist vs a whitelist. A whitelist will always be more secure and less prone to failure.

    A good OOP design should allow you to inject whatever behavior is necessary without having to globally replace your classes all over the program.

    This method, in my opinion, makes the system highly nebulous, and thus less secure. Which is the last thing I’d want from a piece of software that handles sensitive data.

  6. Ilia Jerebtsov on August 27th, 2009 4:47 pm

    More: As an example, you could take your getPrice() method, and instead of overriding its behavior directly, make it internally delegate to a PriceCalculator instance, which can be swapped out as necessary. That way, if you want to change the price logic, you just need to configure into the system which PriceCalculator class to use, hopefully only in one place, instead of having to override behavior for the entire application.

  7. dodger on August 27th, 2009 4:51 pm

    Ilia,

    hooks are simply not enough for eCommerce. We are talking about hundreds up to a thousand of them. Why to add them everywhere and make all show owners suffer just because a few - special and high volume shops - need to tune/overwrite the functionality?

    And for sure OXID won’t guarantee any functionality, why should they? If the customer make changes it’s up to the customer to take care about the correct functionality.

  8. Danne on August 27th, 2009 4:58 pm

    As an enterprise you often need to change functionality in unforeseeable ways. A good plugin API can only get you so far and sometimes the need to change core functionality arises. Developers cannot foresee all use cases and thus making it safe to override behaviours is good.

    There are many things here that point in the direction of AOP. Systems like this might be good candidates for dependency injection. A good AOP framework would probably help out. However, unfortunately not that many developers feel comfortable with AOP in the PHP world. Especially since the support is somewhat questionable in PHP.

  9. Sebastian Bergmann on August 27th, 2009 5:32 pm

    Would be great to meet you in Barcelona, Lars. :-)

  10. Matthew Weier O'Phinney on August 27th, 2009 5:49 pm

    I personally disagree that extension is the correct way to go here. What is described is basically an AOP-style system, and can be accommodated in PHP with a plugin system and/or pubsub system. The code as published is a convoluted hack that makes tracing the application execution very difficult.

  11. Ilia Jerebtsov on August 27th, 2009 6:38 pm

    > Why to add them everywhere and make all show owners suffer just because a few - special and high volume shops - need to tune/overwrite the functionality?

    If a shop needs to alter core functionality to such a point that even a good extensibility API won’t get them where they need, then one can presume that they understand and are experienced enough with the code behind it to know all of the repercussions of changing things in the core.

    At this point, unless OXID is absolutely committed that every one of their classes and methods, internal or otherwise, behaves exactly the same in every version, an update has as much chance to mess something up as if one was to modify the code itself. In fact, it could be worse since an “automated” patch might cause subtle incompatibilities with some module that digs too far into the core, and these errors might not be as easily detected as they would be if any such deep-reaching code had to be reviewed manually.

    It seems to me that if an explicit extensibility system were to exist that could cover, say, 95% of the cases, then the remaining would be these very large and very specific shops, who would have the resources to track changes to the code manually with the use of a version control system. This might not be as convenient to them, but it would make the application more secure, less error prone, and more easily verifiable - and if I own a shop that’s big enough to land in those 5%, I’d take that assurance over convenience any time.

    If one really needs an automated patching system, a tool that does a diff against the source code, verifies that the module package is still compatible, and then replaces code as necessary, might be a better approach. Simple Machines Forum has a great example of this in action.

  12. Stuart Herbert on August 27th, 2009 9:40 pm

    What you did sounds much like the ‘mixin’ language feature from other OO languages. You can achieve this in PHP with Sara’s runkit extension.

    It looks like what you did might have been a bit convoluted, but I can fully understand what you were trying to achieve. I worked on a very successful commercial CMS written in PHP, and in order to support per-client customisations, we did something similar only we used runkit.

    Plugins and modules look great in a textbook or a blog article, but in the real world they are not the solution for every per-customer customisation problem.

    Best regards,
    Stu

  13. Neil on August 27th, 2009 9:54 pm

    PHPTerror is a humorless, pointless DailyWTF wannabe directed toward PHP. It seems like Arno could find something a bit more productive to do than to point and scough at other’s code, or at the very least, try to be a bit more clever or funny about it.

  14. dodger on August 27th, 2009 9:57 pm

    Matthew,

    I agree - but not for OXID. It has to serve to much different approaches. Don’t forget there are shops based on oxid out there which can server 1000+ orders per second. Every tiny little legacy counts. I take the point that it would have been nicer to solve different but taking flexibility and speed into account I still see no other solution. Maybe we meet one day then we should discuss this :)

  15. dodger on August 27th, 2009 10:00 pm

    Stuart,

    we are talking about an out-of-the-box software which runs everywhere. Hosted environments, strange servers with crazy sysadmins etc.

    Runkit? No way. You are right if I would install the software only on my servers and I would have full control - otherwise… no way :(

  16. Arno on August 27th, 2009 11:30 pm

    Hey guys, Hey Neil,

    good discussion going on here, except for you Neil ;)

    Is it that hard for you to acknowledge that a provoking article can lead to a good discussion?

    Or do you think this discussion would have been kicked of otherwise?

    Cheers,

    keep and it up Lars!

  17. Herman Radtke on August 27th, 2009 11:41 pm

    This is a simple case of the ends justifying the means. For the realists this is ok. For the purists this is an abomination.

    Where do I stand? I agree that this method is not ideal. However, no one has given a successful real-world example as evidence there is a better way.

  18. Alan Knowles on August 28th, 2009 3:01 am

    He’s the crux from code reviewers.

    Often as consultants we are asked to advise on solutions based on a client requirement. (go download that code, it’s free and does most of the things we need…)

    Critical to this is the security analysis, and understanding of the code. Both from the perspective of potentially customizing it, and maintaining any customization alongside syncing those changes with any updates in the original code in the future

    From a security perspective, by using this unusually complex pattern, you are making a full code review considerably more complex, as hunting down code paths becomes a fight against magic. - That does not mean the code is less secure, but it’s just very difficult, and more time consuming to judge during a review.

    From a maintainability perspective, we have to trust that you are going to keep classes using this method reasonably similar. - with an interface, visitor or event pattern, this would be considerably cleaner. however multiple layers of inheritance are very susceptible to breakage if any of the elements in the chain are modified (overlapping methods for example).

    Hence I guess this is why you got the general reaction/reluctance to accept this pattern as good coding practice.

  19. dodger on August 28th, 2009 7:08 am

    @Herman,

    very good. I think you pinpointed the real problem. Realist vs. Purist, or Theory vs. Practice ;)

  20. dodger on August 28th, 2009 7:14 am

    @Alan,

    I fully agree. I don’t say that this is the holy gral of software development. There are many drawbacks and you showed a few of them. Nevertheless I still do not see any other real-world-working option ;)

  21. noel darlow on August 28th, 2009 4:26 pm

    How do people learn to code well? That begins by being willing to learn not defending the indefensible, or claiming that bad code is somehow solidly “practical”. That misses the point completely. It’s not difficult to make computers do stuff. The art is in writing code which humans can understand and work with easily.

    Personally I wouldn’t buy anything from a site which I knew was using bad software. I’d find myself asking the question: if they don’t know how to write good OOP code, what else don’t they know? Ecommerce must be done to the highest possible standards or not at all.

  22. Neil on August 28th, 2009 5:34 pm

    Arno - If by a good discussion, you mean dodger having to spend his time defending his efforts under fire, then, I suppose you’ve really accomplished something.

    Something like the good conversation about freedoms and communism Joe McCarthy kicked off in the 1950s.

  23. Steven Morris on September 1st, 2009 6:03 pm

    I’m disappointed that Arno chooses to come around here taking credit for this. This is exactly the kind of discussion that his unprofessional, childish, poorly-written, content-free script kiddie webpage fails to engender. How it got onto Planet PHP is beyond me, and suggests that maybe Planet PHP is not where it’s at any more.

    All credit to you Lars, you’ve been open and honest thoughout this unseemly episode, whilst Arno has done nothing but embarrass himself and his employer. Should Arno himself one day find himself running a large, successful open-source project that doesn’t suck royally (Xinc, anybody?), I feel he may change his attitude.

  24. Arno on September 1st, 2009 11:48 pm

    Hello Steven,

    you just totally disqualify yourself. No need to even get onto that level with you.

    You are definitely not the kind of person that would need any QA processes or a Continuous Integration Server, so I can understand that you are not interested in having a look at Xinc. I won’t blame you for that either ;)

    Wordpress is another big, successful open source project. But big and successful does not mean good source code.

    Anyway, you just don’t seem to get the point of the whole article and the discussion, no blaming, just thinking out loud.

  25. Steven Morris on September 5th, 2009 4:07 am

    Arno -

    Clearly, I can only aspire to join you on your superior, professional level, but to “re-qualify” myself: I’m a published author on the subject of CI. I also make the OSS architectural decisions for a major telecoms provider here in the UK. We use CI intensively. We more than “looked at” Xinc, we analysed it in depth, compared it to the alternatives, and found it severely lacking on all fronts. It’s awful.

    I look forward to the day when some script kiddie starts a snipey little blog about it. Wouldn’t that be ironic?

    FWIW, you yourself completely missed my point about what it will be like for you when you one day have to run a successful codebase yourself. Luckily for me, Lars has argued that corner with aplomb and dignity. You’re not in his league and your childish little site proves that.

    Regards,
    Steve

  26. albert on September 14th, 2009 1:13 pm

    I love the way OXID is structured and I enjoy to write code for this plattform. As a PHP-Programmer for many years now I have seen much worst code than OXID, XT:C for example ..

    Sure, not everything is perfect in OXID. But which code is perfect nowadays?

    So keep up the good work!

  27. Upcoming talks… : Frontalaufprall on September 25th, 2009 4:50 pm

    [...] if you are interested in the topic of the discussion with Arno - feel free to contact me. I will organize a meeting with where we will discuss the whole item over [...]

Leave a Reply