HackerOne users: Testing against this community violates our program's Terms of Service and will result in your bounty being denied.
Addon directory fixes
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?
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'));
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.
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
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_zipwhich should be named
openZip, but seeing method names like
open_zipwhich 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"
Thanks a ton for your feedback R_J!
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 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...
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).
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!
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']);
Another copy and paste error, well spotted! :P
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:
What I didn't read, was that half a page later it 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.