(split) Module compatibility discussion

Discussion in 'Developing, APIs and extending' started by jimkeir, Aug 26, 2012.

  1. jimkeir

    jimkeir Member

    Hi,

    And speaking of __modules, what on earth was behind that particular piece of insanity? I know the word "app" is fashionable, but only developers are going to care what the folder is called and now I need to go through a large, complex bit of code and change every reference to the path so that it's aware of where it is depending on the version.

    So, an unnecessary large coding job, plus double the testing and validation indefinitely, because somebody thought that the name of a folder that few clients will ever see ought to be kewl?
     
  2. Jamie Edwards

    Jamie Edwards Chief Limey Staff Member

    When the terminology changes externally (in the helpdesk, on the site, in the documentation), it would be confusing to new Kayako developers. While making many other framework changes, we decided to take the opportunity to make this change too... sooner rather than later :)
     
  3. jimkeir

    jimkeir Member

    Hi Jamie,

    OK, fair enough I suppose. Just out of interest (and off-topic for this thread - sorry) is there any way of finding out about breaking changes like this before I get site-down tickets from customers? I'm also going to need to change code and documentation, FAQ entries etc. and it would have been nice to have a little warning rather than end up working over a bank holiday to support customers who are unable to take payments. To try and mitigate the immediate impact, would it not have been possible to run both "__apps" and "__modules" for a few months but put a daily warning in the dashboard error log saying that "__modules" is due to be removed?

    Thanks,
    Jim
     
  4. Jamie Edwards

    Jamie Edwards Chief Limey Staff Member

    Hi Jim
    There shouldn't be any breaking changes for modules designed for the framework prior to 4.50 (but if there was something you relied on that we were not aware of, please let us know).

    It isn't possible to do that, I'm afraid.
     
  5. nibb

    nibb Reputed Member

    I agree, why would you change the name from module to apps? Just because its popular? I think the description modules apply better to Kayako. Its not a phone or a OS.
     
  6. hwevers

    hwevers Established Member

    You guys should have thought about the consequences of this renameng for 3rd party module developers. On simple way of saving us a lot of time and making us keep two version would have been to leave the SWIFT_MODULESDIRECTORY definition active and point it to the __apps directory.
    Can you do this for the next release? If we can count on it then we can instruct customers to wait for the next version or direct them to add a line to swift.php and move the complete module from __modules to __apps.
    Not happy!
    Henk
     
  7. Jamie Edwards

    Jamie Edwards Chief Limey Staff Member

    Hi nibb,
    I answered this above:
     
  8. Jamie Edwards

    Jamie Edwards Chief Limey Staff Member

    I'm afraid that isn't possible - you will need to update your path references for 4.50. Sorry if this causes you inconvenience - however, we're sure any disruption would be short lived vs making the change at a later data.
     
  9. jimkeir

    jimkeir Member

    Hi,

    Sorry Jamie; I accept that this isn't going to change but you (Kayako, collectively) need to understand how much extra work you've added to your 3rd-party developers with no reason whatsoever.

    As Henk says, leaving that "SWIFT_MODULESDIRECTORY" definition in place is one line of code for you, but it's no longer there and that breaks a great many things. The whole point of a define such as this is to make modules portable should you decide to rename the directory. If you'd left that in place, then many things would simply work out the box and not now need to be re-written in a rush.

    Would it have helped if we'd decided to use the "SWIFT_Modules" class to work out our modules' base directories? No, because that's no longer thereeither. This makes something of a mockery of the supposed backwards-compatibility offered by using the "official" classes, which was one of the benefits for developers that was touted during the migration from V3 to V4.

    As for it "not being possible" to continue to support SWIFT_Modules for a while and offer a warning in the meantime, try having your developers put this into place in __swift/library/SWIFT_Module/class.SWIFT_Modules.php

    PHP:
    <?php
     
     
    if (!defined('SWIFT_MODULESDIRECTORY')) {
    define('SWIFT_MODULESDIRECTORY'SWIFT_APPSDIRECTORY);
    }
     
     
    $bt debug_backtrace(true);
    SWIFT_Loader::LoadLibrary('ErrorLog:ErrorLog');
    SWIFT_ErrorLog::Create(SWIFT_ErrorLog::TYPE_GENERAL,"Deprecation","__modules directory is no longer supported. Called from ".$bt[2]["file"]);
    class 
    SWIFT_Module extends SWIFT_App {};
    unset(
    $bt);
     
    ?>
    That's all it takes, Jamie, to provide a great deal of backwards compatibility so you might want to question whichever developer told you that it's "not possible". One short file, and one line somewhere central to call it, would have saved me and probably many others many days of unwanted re-work. Two or three extra lines to make sure it only logs once a day.

    For reference, here are some good reasons for 3rd-party code to need to be aware of the installation directory:
    1. The distribution archive may well be from the top level, so the "__modules" or "__apps" directory is in the download. The alternative, having two downloads, will cause confusion for some clients and additional load on support services.
    2. Alternatively, user documentation for installation will need to be updated to tell clients to manually copy files into the right place. This would also cause confusion and extra support load.
    3. Paths to things like private configuration files, shared files not accessed through Kayako's Library system, or module-specific images, will need to be updated throughout the code.
    4. URLs called by external entities will need to change. Some places may not allow URL parameters in callback URLs, so a full URL needs to be given. Also, SWIFT messes up URL parameters and POST parameters in some cases so bypassing SWIFT may be a requirement. This would also require a decision on the part of a potentially non-technical user.
    Additionally, if a developer wants to provide web resources like stylesheets or images with their install then they need to either distribute at the top level (see point 1 above) to deploy to the shared "themes" directory, or store those resources along with the application - which is under the "__modules" or "__apps" directory. Either way, a path containing either "__modules" or "__apps" will be needed. This means that:
    1. Paths in templates will need to change. Customers may have spent a great deal of time, effort and money having these customised and now must do it again.
    2. Paths to icons for menu items which are hardcoded in the templates.xml file need to change. This implies having - and maintaining - two different template files which are loaded depending on whether the path is "__apps" or "__modules".
    So, the result is that if I want to support both pre- and post-4.50 I need to have two different deployments containing both different pathsanddifferent code, and therefore double the test and build effort. Luckily for me, SupportPay is covered by extensive automated tests so I've been able to do this and validate the results without too much effort. Others may not be so lucky, and of course I have no guarantee that there aren't problems in out-of-the-way places which aren't covered in the automated tests.

    This also means that rather than

    the disruption and additional workload will continue for the lifetime of Fusion. From Kayako's point of view it doesn't matter whether a client is running pre- or post-4.50 because everything's distributed as one and is therefore automatically compatible, but that's not the case for 3rd-party providers who will now have to cater for two different and totally incompatible versions.

    It's not the fact that it's been changed that's so galling - it's your application, after all, and you obviously need to do what you feel is best for it. I wouldn't argue with that for a second. What's annoying is that it would have taken so little effort on Kayako's part to completely remove any compatibility problems; what makes it even more annoying is that even those of us who used the 'official' interfaces - that define and class listed above - have still been shafted in this move with no warning whatsoever. A move which has no benefit beyond using a more trendy name for an existing feature.



    I don't know who fed you that nonsense; did you think it through before you repeated it in public? A developer isn't likely to be phased by a line in the docs like "All code relating to Apps should be stored in the __modules directory for legacy reasons.". Users on the other hand, quite often non-technical types who will never see this directory except during installations, are having to work out which directory to use depending on their current version, and facing having their helpdesk broken during upgrades.

    You may not realise that out here in the real world, not everybody automatically upgrades to the newest release as it arrives. Many people won't upgrade unless they have a problem and may be perfectly happy being months or years behind the latest-and-greatest. You have a large population of installations of varying vintages and will continue to have that spread indefinitely, and the "new developer" that you're trying to help has now got to not only get to grips with the SWIFT architecture, but produce two different sets of application code, documentation and installation files. Your "new developer" will suffer exactly the same frustration from this that the "old" developers do. That strikes me as much more confusing for developers old and new, and customers.

    What about all the tutorials and example code that's out there? All the documents and Wiki pages? They're now all wrong. That sounds pretty "confusing to a new developer" to me.

    You want to change "Modules" to "Apps"? Great, go for it. Change the text in the language files and this would have achieved your aim completely as far as customers are aware, plus it would have reduced the workload for Kayako and dramatically reduced the workload for us other developers. The user base, I'm willing to bet, really couldn't give two hoots about what the directories and classes are called.


    Incidentally - and I'm sure you're aware of this - your entire license-decoding mechanism is now open source. It would take someone so inclined about ten minutes to write a key generator, or ten seconds to remove it entirely. Obviously they wouldn't get any support, but it does seem rather redundant to issue licenses now?
     
  10. jimkeir

    jimkeir Member

    Guys,

    This is worse than I'd suspected. SWIFT_Widget is missing from Library probably pre-defined, but if you're explicitly loading it as a Library then it'll break. Other places, they've basically just done a search/replace on the entire app for "module", changing it into "app".

    This means that various method names have changed too - SWIFT_Widget:: DeleteOnModule and SWIFT_Language->LoadModule, for example. The format of the "settings.xml" file, if you load settings during upgrade or install, is also different, again with "module" being changed to "app". God knows how many other places have been touched. The database structure has also changed "module" for "app" in every case, so if you're accessing that directly you'll need to have your SQL tailor itself depending on the version.

    In short, sodding disastrous. If I didn't have the automated testing in place this would have shut me down entirely.
     
  11. masterctrl1

    masterctrl1 Kayako Guru

    Is it wrong to assume all add-ins will use the API at some point in the future and issues like this will be avoided whenever the core platform changes?
     
  12. Jamie Edwards

    Jamie Edwards Chief Limey Staff Member

    Hi Jim,

    I am checking with our developers as to why this provision wasn't made. If it is possible and balanced (in terms of disruption) we will do our best to do it.


    We didn't set out to cause you or anyone upset with the changes, and what I said wasn't nonsense. That is why the change was made; going forward, we will be treating modules very differently and it is an important distinction to be made (although it doesn't look like it now).


    They are in the process of being updated and will be when the main announcement is made.

    Not at all - the code is only available to paying customers, and we trust that they're all honest :)

    Thank you for your feedback and again I am sorry for the disruption caused. I'm seeing what can be done to smooth it over.
     
  13. n8.davis

    n8.davis Established Member


    Maybe a symlink would get you by for awhile????
     
  14. jimkeir

    jimkeir Member

    Hi Jamie,

    Thanks for the reply. As you can probably tell, I'm pretty annoyed about the unplanned extra work this is causing. Nothing personal, honestly.


    In my opinion (and corrections welcome!) this isn't likely to ever be the case. The REST API is fine if what you want to do is interact with existing features; get and create tickets, users, organisations etc. As soon as you start trying to enhance the base system, however, you will need to start interacting with it just like the supplied modules do, and that is with the SWIFT framework classes.

    Let's take a very basic module; you want to create a template for a user screen which will show the count of tickets created in the last week/month/year, with an admin setting for that last option. Just installing it in the first place requires you to override a class. Creating and reading a setting requires you to use a class, as does setting up and rendering the template for the clients. Having that picked up as a valid display requires you to override another class. Let's now say you want to add a scheduled task to count the tickets in the background and store the results. Well, that's another class to override for the task, plus another class to use to create the task in the first place. If you go a step further and consider sending notification emails from that task you need to use more classes, and also maybe load your own language files using another class.

    Except that now, loading the language file is broken because the member function changed from "LoadModule" to "LoadApp". Creating the templates during install failed because you sensibly put the template file with the rest of your code, and it's now moved from "__modules" to "__apps". Fix that and it's still broken because the "module" tag in the XML that describes templates also changed to "app". Same two problems for creating settings.

    Yeah, I thought of that for a while but with so much else changed - class names, method names, file formats - it wouldn't be a complete fix. I'm still probably going to end up creating a deployment file with identical files under "__apps" and "__modules" and it will certainly help with the images/css and direct callbacks while I try and find a sustainable way of moving them around. A symlink would be better, of course, but you can't put a symlink in a zip archive to keep the complexity down for customers. I suspect the long-term solution is to modify all the templates - 30 of them - and the code that calls them, to pass in the correct path for these elements as a template variable.
    Jamie, if I might make some suggestions:
    • You have a load of developers registered on Forge, complete with email details. Where breaking changes are concerned, it would be worth considering emailing these registered developers with a beta copy a short time before release to allow them to test. This improves the quality of the product overall from the customers' perspective (i.e. fewer "I upgraded and everything died" calls) and completely removes any excuse we would have for ranting about unexpected changes.
    • Most companies give their contributing community some time to deal with breaking changes with things like deprecation warnings. Adding that stub class and the define outlined earlier would make simpler modules (i.e. no templates or settings) work with few if any changes, but still generate warnings so that the developers find out that changes are required. Six months from now if you're feeling uncharitable, remove those calls; better, leave them in until V5 when you have every justification for massive, breaking changes.
    To complete this transition gracefully it would be required to:
    • Add the SWIFT_MODULESDIRECTORY define back in.
    • Create that SWIFT_Module stub class.
    • Add stub functions for every case where you've renamed an existing method from "SomethingModule" to "SomethingApp"; there probably aren't that many, and in each case the code would be trivial e.g. (pseudocode)
    Code:
    /* DO NOT USE THIS METHOD, IT WILL BE REMOVED */
    public function SomethingModule($param1, $param2) {
      SWIFT::Deprecated(__FUNCTION__);
      return self::SomethingApp($param1, $param2);
    }
    
    • When loading templates and settings from XML, check that the array being processed has an "app" attribute in the right place. If not, deprecation warning again and revert to "module". Same principle; Fusion continues to operate, 3rd-parties get warned but not burned, you have a total of about 3 lines of code to add and a scattering of array references to modify.
    Points one and two are already done in effect, you'd only need to add the file. Points 3 and 4 would take a little more effort but really not much, and would be much more efficient overall for Kayako to add code in a dozen or so places than for all your 3rd-party contributors to modify calls to that code in potentially hundreds. You still get to phase the old naming out, but we get a decent transition period in which to do it instead of just "site down".

    I'll even make the changes for you if you want. For a fee, naturally.
     
  15. Jamie Edwards

    Jamie Edwards Chief Limey Staff Member

    I have asked a couple of our devs to look into this today.

    Given that the intention was the preserve backwards compatibility, I believe this was an oversight and skipped over because it was part of the renaming of that directory.

    Yep - hands up, we definitely should have done this. I'll make sure this surprise disruption doesn't happen again. We are currently formalising this process where we'll be providing early access and warning to developers and customers interested in betas. You'll receive some information about that shortly.

    As above... supporting and making life easy for devs is one of our top priorities (even though the changes in 4.50 might make it look otherwise!). We'll live up to it.

    We are looking into these now, appreciate the detailed suggestions.
     
  16. jimkeir

    jimkeir Member

    Hi Jamie,

    Thank you for looking into it.

    I have come across many other areas where things have been broken in the last few hours. Some details here; if you'd rather continue by email, or a support ticket, please let me know.

    • All "MODULE_xxx" definitions i.e. MODULE_LIVECHAT have also changed to APP_LIVECHAT . Both definitions can co-exist.
    • The "IsRegistered" member function of SWIFT_Module no longer exists in SWIFT_App, so all places which check for existence of a module now fail. This is essential for tailoring bits of a module according to whether tickets and/or live chat is available, since many classes and database structures are different.
    • SWIFT_Widget has been removed from the Library, so any attempt to LoadLibrary('Widget:Widget') fails.
    • Ditto for SWIFT_User, SWIFT_Template and SWIFT_UserGroup. This last lot and quite a few more seem to be "Models" now, so calling LoadLibrary now barfs with "File missing" errors. Again, stub files in the Library directories which load the Model and issue a deprecation warning would fix this and be removable in the future at your choice.
    I'm now on just short of 130 files modified so far and still without a running system.
     
  17. Jamie Edwards

    Jamie Edwards Chief Limey Staff Member

    Hi Jim,
    Unfortunately we won't be able to make these changes. Your customers will need to hold off from upgrading until you update your modules for compatibility with 4.50.
    Meanwhile:
    • I have updated the changelog to include even more prominent warnings around the breaking changes
    • We have added a note to my.kayako.com where customers download the latest product files
    • We are working on more thorough documentation for developers around these changes
    • As discussed I will make sure we don't have this communication problem again with third party devs
    • I have emailed you the contact details of two of our senior developers who you are free to contact if you run into any issues while updating your apps
    Once again, I apologise for the disruption caused - we dropped the ball and should have been more proactive. These changes had to be made in this update, but without a doubt we should have done better to preempt these issues. This won't happen again!
    The reasoning and benefits of the framework changes made in this update will become more clear in the near future, if they're not clear right now.
     
  18. hwevers

    hwevers Established Member

    OK, took me a day to get my modules working and compatible with all 4 versions of Fusion. To save you some time here is what I found out (and allow me to grumble about this update that totally ignored us 3rd party developers):

    SWIFT_MODULESDIRECTORY is missing instead of pointing to the __apps directory. Now, I don't know about you, but I'm no longer going to use any defined path from Fusion. So I now use something like:
    $includefile = dirname( __FILE__ ).'/../library/includes/'.$this->integrationName.'/class.SWIFT_BREIN_Integration.php';
    This will make your module compatibloe with all version 4 Fusion code wherever they may move the directory to.
    As long as you know where your PHP file is located you can easily calculate the path you need. I'm not yet sure if this breaks on a windows server. I probably should use DS in stead if the forward slash.
    grumble 1: in stead of deleting this constant the developers could have left it in and point it to the __apps directory: Time needed: hmmm 30 seconds seems long. Getting the idea to do this for us seems to be the problem.

    Your configuration settings.xml file will throw an error on 4.5 because in this XML file the word "module" has been changed to "app". You can make this compatible by leaving the module and add the app field:
    <?xml version="1.0" encoding="UTF-8"?>
    <swiftsettings>
    <group name="settings_BREIN" app="breincoach" module="breincoach" ishidden="0">
    grumble 2 : 2 minutes thinking (and that's slow) and 2 minutes work to make the Fusion code accept both. Getting the idea to do this for us seems to be the problem.

    Many classes are now preloaded. I understand that. Great. However the code for loading a library fails for these preloaded libraries.
    Have a look at __swift/apps/base/models to find out what is preloaded (no confirmation if I am correct until now, but this seems to be true).
    In my case the complete user library moved from library to here. So do not load all these classes. Njet. There are still some left in the library __swift/apps/base/library(/users).
    Those are not preloaded I guess.
    So you need to figure out which library need to be loaded. To make it pre 4.5 compatible I have done this in my classes:
    public function __construct(){
    parent::__construct();
    if (!class_exists('SWIFT_User')) {
    $this->Load->Library('User:User', false, false);
    }
    if (!class_exists('SWIFT_UserEmail')) {
    $this->Load->Library('User:UserEmail',false,false);
    }
    SWIFT_Loader::loadlibrary( 'debug:BREIN_DebugLog','breincoach');
    SWIFT_Loader::loadlibrary ('magic:BREIN_Magic','breincoach');
    $this->Language->Load('users');
    $this->Language->Load( "breincoach", "breincoach" );
    $this->Load->Library('Settings:Settings');
    $this->integrationName = strtolower(trim($this->Settings->Get('BREIN_external_integration')));
    return true;
    }
    grumble 3 guess...

    Ok, Am I pissed off? Not any more, this .. sometimes happens. I got warned by a customer early and I now have new compatible versions on my website. Needed to update the manuals as well, especially the installation instructions. Even the name of my main module, now being an app, had to be changed to not confuse the customers So FUIM is now an acronym for Fusion Ultimate Integration App. Hmm...

    Ok, I sincerely hope this will be a learning moment for the Kayako developers. Please put a "what will the impact of our changes be on 3rd party developers and how can we minimize those" in your design diagram.
    Thanks
    Henk
    www.breincoach.eu
     
    Jamie Edwards likes this.
  19. Jamie Edwards

    Jamie Edwards Chief Limey Staff Member

Share This Page