ILIAS-eLearning
Introduced new update parameter for plugged page content
In ILIAS 6 a plugin could determine whether the content was moved on the COPage or deleted by accessing ilCtrl's command (getCmd()) when ilPageComponentPlugin::onDelete() was invoked. In ILIAS 7 this functionality got lost due to the new action handling and the plugin cannot tell whether the object is moved or deleted anymore. This is necessary because a plugin must know if it should delete content from the database or not. Therefore I introduced a new update parameter which is passed to ilPCPlugged::handleDeletedPluggedNode(). Instead of passing on this parameter to ilPageComponentPlugin::onDelete() I added it to the properties array, so that plugins aren't incompatible when this PR gets merged.
I've also opened a mantis ticket since the behaviour was (though as a workaround) possible in a previous version: https://mantis.ilias.de/view.php?id=34328
The parameter $a_update in deleteContent just tells the method if the page update method should be called or not. Not if a move or delete operation is currently performed. These are two different things, even if they currently may be "in synch" by coincidence.
It seems more questionable to call $this->handleDeleteContent($curr_node); if things are only "moved" at all.
Thinking about it, I wonder how this can work, if users use the clipboard. They could just remove content and never paste it. Maybe we need proper events for all of this, cutToClipboard, pasteFromClipboard, delete, move.
Anyway, I can't accept this PR, since the $update flag servers another purpose.
Hi @alex40724
As far as I understand, there are already different commands for this in PageCommandActionHandler. So it would already be possible to distinguish temporary from actual deletes.
The problem you mentioned by misusing the $a_update flag makes sense. What I could do then, is introduce a new say $a_update_db flag which can be set accordingly when calling the method deleteContents(). That wouldn't address the issue that it doesn't make sense to call handleDeleteContent() in ilPageObject, but it would at least prevent plugins from permanently deleting their contents by accident.
What about adding a "move" flag and not call handleDeleteContent for these cases?
I'm not quite sure what handleDeleteContent() does, but if I understand correctly it's not responsible for removing the element on the page right? Because then the move-flag approach would make more sense and even get rid of the properties entry.
@alex40724 I remove the handleDeleteContent() in ilPageObject::deleteContent too because this method was only called for move-events.
@thibsy Thanks for the change. I am not sure, if this is the best way to move forward. I need some time to check alternatives. But this is currently not top prio for me (neither highly voted bug, nor paid support).
@thibsy Thanks again for the PR. Again I spent some time on this and decided keep the existing calls, but add an explicit additional parameter for move operations, see https://github.com/ILIAS-eLearning/ILIAS/commit/b671d431281617d6387522ffa56720656059d69c
This is 8 only, since it is incompatible with the old interface. Since this information was not part of the old interface, plugins still need to use workarounds in ILIAS 6/7.
@ Jourfixe: The onDelete method in page plugins gets a new third parameter, which is set, if the deletion is part of a move/cut/paste operation:
https://github.com/ILIAS-eLearning/ILIAS/commit/b671d431281617d6387522ffa56720656059d69c#diff-0b8983602055b0140f6b49204099f7dad4159cb3f9cf48f846737f7befafd70e
Thx @alex40724 for looking into this. What should plugins for ILIAS<8 do, to know whether its a movement or not? Which query-params can be used?
@thibsy There is no supported way to do this. Only workarounds. For 7 POST["component"] and POST["action"] can be used.
Hi @alex40724, I'm sorry if I'm a little stubborn on this.
In ILIAS 6 there's a workaround where the requested command can be checked for cut. in ILIAS 7 this isn't possible anymore, because there is no POST request (or it would be after the initial GET request) and the GET parameters only provide invokeServer as the current command. Is there any other workaround?
IMO we should at least be able to provide a (sort of) official workaround which plugins can use, because otherwise plugins that store content in the database aren't able to tell if a content can be safely deleted. This means content-page plugins in ILIAS 7 can only insert into the database, and because of the way content-pages are structured, latter versions would also not be able to tell which contents are still in use. This means there are plugins that produce data that cannot be removed anymore.
I do not understand the problem with the fetch requests and their POST information. Of course they are after they initial GET request. This request only sets up the editor. But the requests include the same information, e.g. cut.
ILIAS 7 Plugins can implement the same sort of workaround as in ILIAS 6.
Okay @alex40724, I've found the issue: ILIAS\COPage\Editor\Server\Server::reply() does not rewind the current request-body's FileStream which leads to plugins having to rewind it first in order to retrieve the information they need.
I think this should be addressed in ILIAS, therefore I created a mantis ticket here: https://mantis.ilias.de/view.php?id=35576.
@thibsy No, I will not adress this in COPage this way as there is no such rule in any documentation. If this is a must, the request class should already take care of this imo.
More important, as written, these are all unsupported workarounds. COPage does not intend other code to read its requests. ILIAS 8 will get the official documented parameter to solve this, if the JF accepts the change.
Fair enough.
Just FYI and for anybody that runs into a similar issue with their plugins, I worked around this issue in ILIAS 7 by using the following snippet in the plugins ilPageComponentPlugin::onDelete() hook:
global $DIC;
$DIC->http()->request()->getBody()->rewind();
$body = (!empty($_POST)) ?
$DIC->http()->request()->getParsedBody() :
json_decode($DIC->http()->request()->getBody()->getContents(), true);
if (isset($body['action']) && 'delete' === $body['action']) {
// delete your db content
} else {
// don't delete your db content
}
Change is documented here: https://docu.ilias.de/goto_docu_pg_56942_42.html "Changes in ILIAS 8"
Jour Fixe, 14 NOV 2022 : Alexander notified the JF about the introduction of a third parameter $move_operation with ILIAS 8 as a reasonable solution for the plugin interface. We highly appreciate this suggestion and accept the PR for 8 and trunk. For extended functionality of the page content plugin interface, a workshop would be strongly recommended.
