Blog Documentation Try Vanilla Cloud
HackerOne users: Testing against this community violates our program's Terms of Service and will result in your bounty being denied.
Options

Addon directory fixes

CaylusCaylus ✭✭
in Community Addons & Development

Hey guys,

The addon directory has some issues:

  • If you go to an addon download page, the link to the author is broken (because the output of UserUrl is needlessly escaped again, even though userURL already sanatizes dangerous input).
  • It completely ignores addon.json files in your zip file and it requires a PluginInfo array in your plugin file (even though that approach is deprecated). See this issue: https://github.com/vanilla/community/issues/135
  • If your main class is class.Fancyplugin.php, it fails because it requires either class.FancyPlugin.plugin.php or default.php
  • Plugins, applications and themes are now all addons. But there is no general addon type, so even if the addon directory would parse the addon.json file correctly, it wouldn't work because it has no type to file it under.

I think I fixed these various issues, but does someone have time to try out the fix / check the style requirements before I create a pull request?
https://github.com/Caylus/community

Comments

  • Options
    R_JR_J Ex-Fanboy Munich Admin

    Thanks for all your efforts!

    You should package your commits into "fix/some-error" and "feature/great-thing" branches. The bigger your commit is, the more time it will take to be merged, since Vanilla team would have to look at the complete PR as such.

    e.g. your "UserUrl is already sanitized" commit would be a great "fix/double-sanitation" branch.
    In the text you could show both relevant lines:
    $author = $this->data('Official') ? t('Vanilla Staff') : userAnchor($this->Data, null, array('Px' => 'Insert', 'Rel' => 'author'));
    echo htmlspecialchars($author);
    adding that function userAnchor already uses $text = val('Text', $options, htmlspecialchars($name)); which leads to a double sanitation.

    The advantages are that small fixes will be merged faster because it is easier to see their impact.

  • Options
    R_JR_J Ex-Fanboy Munich Admin

    You have added a new addon type "general" which sounds to me like "something which is fuzzy enough to make it work". Whatever the reason for the decision has been should be documented in the commit or even better in a branch "feature/new-addon-type" branch

    I really would advice to start by doing small PRs and see what the reaction from Vanilla team would be. You have used a requireone and I'm pretty sure that this will not be accepted, but I don't know the correct way to include the class...

    Oh wait... some commits later (or earlier) I see that you have created that class. The autoloader should include that class. Have you tried disabling the addon, clearing the cache, enabling the addon and then simply try a $addonInfoExtractor = new AddonInfoExtractor ();

    Feedback to that class:

    Either this is in the constants file or it is a class constant, but you shouldn't ensure it in this way. If you insist on doing so, add a space before the 6

    if (!defined(ADDON_TYPE_GENERAL)) {
    define("ADDON_TYPE_GENERAL",6);
}

    Instead of throw new Exception("$path not found.", 404); use throw notFoundException('Path');. A 404 error is a http page not found error code. Was that your intention?

    There is a mthod open_zip which should be named openZip, but seeing method names like checkRequiredFields and open_zip which would apply to old and new style plugins, I would prefer to either see one class handling both or having three classes where two of them are extending the third which handles the file IO stuff

    Your new addon.json shows the description "The addon management tool for VanillaForums.org.". There is no name for this community but it might be better to use "open.vanillaforums.com"

  • Options
    CaylusCaylus ✭✭

    Thanks a ton for your feedback R_J!

    @R_J said:
    You have added a new addon type "general" which sounds to me like "something which is fuzzy enough to make it work". Whatever the reason for the decision has been should be documented in the commit or even better in a branch "feature/new-addon-type" branch

    This is indeed correct. I'll specify it's because of this problem in the final version:

    Plugins, applications and themes are now all addons. But there is no general addon type, so even if the addon directory would parse the addon.json file correctly, it wouldn't work because it has no type to file it under.

    I really would advice to start by doing small PRs and see what the reaction from Vanilla team would be. You have used a requireone and I'm pretty sure that this will not be accepted, but I don't know the correct way to include the class...

    Oh wait... some commits later (or earlier) I see that you have created that class. The autoloader should include that class. Have you tried disabling the addon, clearing the cache, enabling the addon and then simply try a $addonInfoExtractor = new AddonInfoExtractor ();

    I believe how it's normally done is not to use require_once, but to simply add the class definition to the end of class.addoncontroller.php file. I just got tired of scrolling all the time, and split it as a temporary thing :P Autoloader didn't include the class, for some reason.

    The problem with the small commits is that for the small issues that works, but that addon.json files are not read really requires a massive overhaul...

    Feedback to that class:

    Either this is in the constants file or it is a class constant, but you shouldn't ensure it in this way. If you insist on doing so, add a space before the 6

    if (!defined(ADDON_TYPE_GENERAL)) {
    define("ADDON_TYPE_GENERAL",6);
}

    This is a temporary measure. It's intended to go in constants.php in the main Vanilla release (where the other ADDON_TYPE_X are defined).

    Instead of throw new Exception("$path not found.", 404); use throw notFoundException('Path');. A 404 error is a http page not found error code. Was that your intention?

    To be completely honest, that part is just a copy paste from UpdateModel. throw notFoundException('Path'); is better? Then I'll change it to that!

    There is a mthod open_zip which should be named openZip, but seeing method names like checkRequiredFields and open_zip which would apply to old and new style plugins, I would prefer to either see one class handling both or having three classes where two of them are extending the third which handles the file IO stuff

    I'm not sure I'm understanding this point. They are handling both old and new plugins.
    The only methods in which they differ is to convert the file into an array, in this line 84:

    $info = ($entry['Name'] === '/addon.json') ? self::addonJsonConverter($entry['Path']) : UpdateModel::parseInfoArray($entry['Path']);

    Your new addon.json shows the description "The addon management tool for VanillaForums.org.". There is no name for this community but it might be better to use "open.vanillaforums.com"

    Another copy and paste error, well spotted! :P

  • Options
    LincLinc Detroit Admin

    @Caylus said:

    • Plugins, applications and themes are now all addons. But there is no general addon type, so even if the addon directory would parse the addon.json file correctly, it wouldn't work because it has no type to file it under.

    I don't understand this. Every addon should have one of the 4 defined types to work in Vanilla (locale, theme, plugin, application). If you don't have the information to file it correctly, we should remedy that, not add a fuzzy addon type to sidestep the issue.

  • Options
    CaylusCaylus ✭✭

    @Linc said:

    I don't understand this. Every addon should have one of the 4 defined types to work in Vanilla (locale, theme, plugin, application). If you don't have the information to file it correctly, we should remedy that, not add a fuzzy addon type to sidestep the issue.

    Ah, then I misunderstood the new system.

    In this part of the docs they say:
    https://docs.vanillaforums.com/developer/addons/#addons-and-plugins-and-themes-oh-my

    Before Vanilla 2.5 there were plugins, applications, and themes. Each had varying functionalities and capabilities. Starting in Vanilla 2.5 all 3 of these have been combined into 1 format - Addons. An addon is capable of doing everything that each those 3 used to do. Existing plugins, themes, and applications are treated as addons in Vanilla 2.5.

    What I didn't read, was that half a page later it said:

    A theme is a special type of Addon, with some additional facilities provided in order to to simplify development. It is placed in a top level folder that gets symlinked into the themes directory of a Vanilla Forums installation. Its information is defined in an addon.json file with its type key set to theme. Older themes may define $ThemeInfo in an about.php file.

    But then it's not clear to me: Applications and Plugins now both have "type":"addon" in their addon.json file right?

    I guess Applications can be detected by their "priority": 10?

  • Options
    LincLinc Detroit Admin

    @Caylus said:
    But then it's not clear to me: Applications and Plugins now both have "type":"addon" in their addon.json file right?

    I guess Applications can be detected by their "priority": 10?

    That's basically accurate, yes. What's really happening is we've stopped delineating between 'application' vs. 'plugin' and are just calling them 'addons'. There used to be real differences between them, but the priority order is basically the only difference left at this point and we hope to remove the concept of 'applications' in the (distant) future. We recommend all new addons just use the 'plugins' folder and use those standards from now on.

    In this new worldview, 'theme' and 'locale' are basically special sub-types of addons.

Sign In or Register to comment.