Skip to content

Conversation

splitbrain
Copy link
Collaborator

This is the new extension manager as started by @michitux at the hackfest. I continued work on it and brought it to a state that's usable and mergable in my opinion. Note: this is a complete rewrite and just reuses the look'n'feel of Piyush's project.

Things that should be done before merging (preferable by people other than me):

  • code review. I might have done something stupid. especially security should be rechecked
  • real life tests. throw shit at it and see if it breaks
  • re-add RTL styles. I threw them out to keep things easy when reevaluate the needed CSS (sorry @selfthinker)
  • cross browser tests
  • remove the old plugin manager
  • whatever I forgot

start test wiki

michitux and others added 30 commits August 1, 2013 11:36
This uses a lot of code and the whole design from the previous extension
manager implementation.
…to extension_manager

* 'extension_manager' of github.com:splitbrain/dokuwiki:
  Extension manager: Implement extension table
  Extension manager: implement uninstall, canModify and install/update
  Extension manager: Improve update check
  Extension manager: Use getInfo() when no info.txt is available
  Extension manager: fix install dir for templates
  Extension manager: add language file and simple admin component
  Extension manager: implemented more extension info and basic repository access
  Extension manager: First draft of the extension class
this is not a extension specific cache but a global one. no need to
purge for each installed extension
@splitbrain
Copy link
Collaborator Author

@selfthinker I can't reproduce it with Firefox and git installed plugins either. it's probably some interfering script from a different plugin.

@selfthinker
Copy link
Collaborator

Ah, I got it! In order to make it easier to fix HTML errors, I switched the template to be served as application/xhtml+xml and then forgot about it. That usually brings about some JS errors. After I switched back, the error disappeared. :)

And that tmp.innerHTML line is from jQuery, so the issue could still originate from the extension manager plugin (it most probably does, but only in XHTML).

}
$this->setExtension($this->getID());
$this->purgeCache();
}catch (Exception $e){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try/catch not needed

@dom-mel
Copy link
Collaborator

dom-mel commented Jan 8, 2014

From just reading the source, the extension manager makes a very good impression :-)

I put all my comments in the source - but havn't test it ;-)

@dom-mel
Copy link
Collaborator

dom-mel commented Jan 8, 2014

The dis-/en-abling of testing plugin feels wrong.

Reproduce 1:

  1. Open the extension manager
  2. Click on "enable" at the testing plugin
    DokuWiki will show "plugin is enabled message", but the plugin:
  • The plugin still looks disabled
  • The plugin still has the enable button
  • The "testing plugin is enabled and should be disabled"-message is still not visible.

When you reload the page it's OK again. Disabling is the same behavior.

they were just catching and rethrowing
PHP_EOL is platform dependent, so you get in trouble while migrating
between platforms.
@dom-mel
Copy link
Collaborator

dom-mel commented Jan 8, 2014

The search has no "nothing found" text.
Additionally the search could be a little bit better. I.e. searching for PDF shows no plugin at all, but there are plenty of plugins with pdf in the title.

The filename found in the URL will be used for old plugins missing a
base entry in their plugin.info.txt and lacking a subdirectory inside
the archive as well. This patch makes sure possible query strings aren't
included in the filename.

Note: io_download() will also try to get a filename from any
content-disposition header.

If no filename can be found we simply use an md5 sum of the URL and hope
the plugin will contain it's own hint for naming.
@dom-mel
Copy link
Collaborator

dom-mel commented Jan 8, 2014

IMHO it would be nice to close the screenshot overlay with the esc key

@dom-mel
Copy link
Collaborator

dom-mel commented Jan 8, 2014

maybe i can do some x-browser tests in the next days. (downloading the testing vms atm)

@splitbrain
Copy link
Collaborator Author

hey @dom-mel thanks a lot for your review.

  • I changed a few things already
  • The search not finding PDF was because MySQL's fulltext search requires 4 chars minimum by default. I reconfigured it on our server.
  • ESC for closing the lightbox would be nice, but I'm too tired to figure out how to do it right now (I'm using a very minimalistic aproach here, so we would need to code that ourselves)
  • I can't reproduce what you describe regarding the testing plugin. It enables and disables just fine and like all the other plugins.

@dom-mel
Copy link
Collaborator

dom-mel commented Jan 11, 2014

Hi @splitbrain,
The thing i described seems to be fixed partly.
The only thing that's left can be reproduced by:

  • Enable testing plugin. - A Warning will be shown at the top.
  • Disable it again. - The warning is still present but the testing plugin is disabled. The warning goes away after the next page reload.

see:
message

@splitbrain
Copy link
Collaborator Author

I think we can live with that since the testing plugin is not part of the
regular release anyway

@selfthinker
Copy link
Collaborator

The testing plugin is part of the regular release.
But I think we can live with it anyway, as it is disabled by default.

@splitbrain
Copy link
Collaborator Author

really? it shouldn't. it makes no sense without the test suite.

@splitbrain splitbrain merged commit b1e7580 into master Jan 19, 2014
@splitbrain splitbrain deleted the extension_manager branch January 19, 2014 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants