Fix can update item calls#23923
Conversation
SebSept
force-pushed
the
fix_canUpdateItem_calls
branch
from
0d91792 to
b4bafec
Compare
AdrienClairembault
left a comment
AdrienClairembault
left a comment
There was a problem hiding this comment.
Seems fine to me 👍
There was a problem hiding this comment.
I need to take time for a precise review, to see if it will not cause issues in some specific case, for instance with a profile assigned to the simplified interface.
Anyway, if you used rector for this, then please propose a PR in our own rector extension (https://github.com/glpi-project/rector-glpi), with relevant tests. This will permit to detect unexpected uses of these methods in future PRs, and will also permit to plugins developers to use it in their plugins.
I'm not really sure about the custom rule. The changes made by the rule need to be reviewed before submitting, however, I'll submit the rule 👍🏼 |
SebSept
force-pushed
the
fix_canUpdateItem_calls
branch
from
b4bafec to
5961014
Compare
Are some replacement invalid ? Is there a condition that can be used to identify them ? |
SebSept
force-pushed
the
fix_canUpdateItem_calls
branch
from
5961014 to
898c83e
Compare
Doesn't deserve to be in rector.php because it sometimes generate redundant code (e.g. !ProjectTask::canUpdate() || !$projecttask->can($projecttask->getID(), UPDATE) ) . Reapply "rector rule to replace canUpdateItem()" This reverts commit b4bafec.
# Conflicts: # ajax/webhook.php # Conflicts: # src/KnowbaseItemTranslation.php
something like `!ProjectTask::canUpdate() || !$projecttask->canUpdateItem();` becomes `!ProjectTask::canUpdate() || !$projecttask->can($projecttask->getID(), UPDATE);` which is redundant and should be just `!$projecttask->can($projecttask->getID(), UPDATE)` Maybe I can fix the rector rule ...
This reverts commit 0cd250f.
SebSept
force-pushed
the
fix_canUpdateItem_calls
branch
from
28e24ae to
a0eda74
Compare
we'll discuss about that in the PR (not yet issued) with the rector rule. I can apply the same for sibbling methods (canViewItem()) in another PR. |
| foreach ($iterator as $data) { | ||
| $item->getFromResultSet($data); | ||
| if ($item->canViewItem()) { | ||
| if ($item->can($item->getID(), READ)) { |
There was a problem hiding this comment.
Same here, we are looping on a single itemtype so it will be more efficient to call canView before the loop and canViewItem inside.
There was a problem hiding this comment.
canViewItem() doesn't check is item is private.
by using can($item->getID(), READ), the code change add the check for isPrivate() which is a good thing.
|
|
||
| // Check rights | ||
| if (!$item->canUpdateItem()) { | ||
| if (!$item->can($item->getID(), UPDATE)) { |
|
|
||
| $item = $document_obj->fields; | ||
| $item['_can_edit'] = Document::canUpdate() && $document_obj->canUpdateItem(); | ||
| $item['_can_edit'] = $document_obj->can($document_obj->getID(), UPDATE); |
|
|
||
| $project->fields = $subproject; | ||
| $item['_readonly'] = !Project::canUpdate() || !$project->canUpdateItem(); | ||
| $item['_readonly'] = !$project->can($project->getID(), UPDATE); |
Checklist before requesting a review
Please delete options that are not relevant.