search

michitux / dokuwiki-plugin-move

Move and rename pages and namespaces whilst maintaining the links
http://www.dokuwiki.org/plugin:move
GNU General Public License v2.0
40 stars 20 forks source link

Add a plugin.move.rename XMLRPC method. #242

Open rotdrop opened 1 year ago

rotdrop commented 1 year ago

The code is extremely simple. It reuses the renameOkay() function from action/rename.php.

There is an open draft pull request #232 which ATM does not contain any real code.

This commit just adds one "rename" remote call. It seems to work. YMMV.

michitux commented 1 year ago

Thank you very much for this pull request! Overall, this looks good and useful, I assume you have tested that this actually works?

Just to be sure, the check for renameOkay is to ensure the group membership (everything else is also checked by the move operation)?

I'm wondering if it wouldn't make sense to make the generated error message(s) available in the response. Otherwise, it will be very hard to display a meaningful error message in the client. If you think this is too much for this first version, what about just providing a generic error message such that more specific messages can be provided later without breaking the API?

Regarding the method name, maybe it would make sense to use something like renamePage to make it clearer that this is about pages and not media files (for which a similar method could be added later)?

michitux commented 1 year ago

I've just had a look at the documentation for remote plugins and I'm wondering if this isn't missing a _getMethods method.

rotdrop commented 1 year ago

I've just had a look at the documentation for remote plugins and I'm wondering if this isn't missing a _getMethods method.

It is not. I was wondering also and had a look at the source code: nowadays you may provide the method, but the default is to use PHP reflection classes and just export all public method (which do not start with an underscore). So really nothing more has to be done. I invite you to just try!

So:

Kind thanks for looking into my pull request!

I must admit that I was really astonished how super-simple the remote extension can be realized! And a Wow! to the DW developers for making it that super simple!

splitbrain commented 1 year ago

Regarding the method name, maybe it would make sense to use something like renamePage to make it clearer that this is about pages and not media files (for which a similar method could be added later)?

I agree. The method name should be adjusted. Otherwise LGTM :+1:

rotdrop commented 1 year ago

Regarding the method name, maybe it would make sense to use something like renamePage to make it clearer that this is about pages and not media files (for which a similar method could be added later)?

I agree. The method name should be adjusted. Otherwise LGTM +1

I'll eventually adjust the name, I understand the point. Thank you for the review!

rotdrop commented 1 year ago

Regarding the method name, maybe it would make sense to use something like renamePage to make it clearer that this is about pages and not media files (for which a similar method could be added later)?

Is there already such a functionality in the move-plugin? exposing it to xmlrpc would probably easy, but having a quick look at the code I could not immediately identify such functionality in the move addon. Hints are be very welcome! Maybe I have just overlooked it.

splitbrain commented 1 year ago

Media can be renamed and moved at least via the tree manager. Adding a rename mechanism in the media manager is #209

rotdrop commented 1 year ago

Media can be renamed and moved at least via the tree manager. Adding a rename mechanism in the media manager is #209

Great! After this has been implemented in the frontend I'll be happy to add it to remote.php.

michitux commented 1 year ago

Media can be renamed and moved at least via the tree manager. Adding a rename mechanism in the media manager is #209

Great! After this has been implemented in the frontend I'll be happy to add it to remote.php.

Why do you want to wait for the frontend? The method for renaming media exists and should be as easy to use as the one for pages:

https://github.com/michitux/dokuwiki-plugin-move/blob/f2416147498d9b33e36445820b54e015f1968776/helper/op.php#L232

rotdrop commented 1 year ago

Why do you want to wait for the frontend? The method for renaming media exists and should be as easy to use as the one for pages:

https://github.com/michitux/dokuwiki-plugin-move/blob/f2416147498d9b33e36445820b54e015f1968776/helper/op.php#L232

Hey! That's the line of code I was searching for. Thanks!

rotdrop commented 1 year ago

Why do you want to wait for the frontend? The method for renaming media exists and should be as easy to use as the one for pages: https://github.com/michitux/dokuwiki-plugin-move/blob/f2416147498d9b33e36445820b54e015f1968776/helper/op.php#L232

Hey! That's the line of code I was searching for. Thanks!

What about access control? For pages I was just re-using the renameOkay() from action/rename.php.

There is a function checkMedia() in op.php which might "be it". OTOH, there is also a function checkPage(). That one is at least not used by action/rename.php. Looking closer: checkXXX() is used in moveXXX() which bails out in case of return false. However, the action plugin action/rename.php has in addition that renameOkay() method.

Question: Do I need that renameOkay() method, or does it suffice to rely on the checkXXX()?

rotdrop commented 1 year ago

A more cosmetic point: as op.php uses moveXXX() as name, maybe the XMLRPC calls should also be named that way -- just to get rid of the slight irritation why at one location it is called "rename" and at another "move". Not so important ...

rotdrop commented 1 year ago

So there it is ... move media is completely untested from my side. But yes: as moveMedia() exists it was really straight-forward ...

michitux commented 1 year ago

What about access control? For pages I was just re-using the renameOkay() from action/rename.php.\n\n

From what I've seen, the only difference between renameOkay and the check in the Op class is that renameOkay() also checks if the user or a group of the user is listed in a special rename allow option:

https://github.com/michitux/dokuwiki-plugin-move/blob/56d59e22e819689a97881bf82d162e484c27a047/action/rename.php#L128-L129

This check isn't used for the move manager in the admin area as that one is already restricted to managers or admins permission (haven't checked which one). The help of this option says explicitly that this is for pages, so not sure it is a good idea to re-use that for media files. I see three options:

I think I would keep things simple and just use the third option or re-use the option from pages and update its description.

@splitbrain any opinion on the right check?