Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generic asset/model/entity controller #18052

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Pierstoval
Copy link
Collaborator

No description provided.

Copy link
Member

@cedric-anne cedric-anne left a comment

Choose a reason for hiding this comment

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

This controller will probably be common to not only assets but most of the CommonDBTM classes. The technical names should not contains asset. Also, it could be considered as an index controller for the corresponding class.

  • Glpi\Controller\Asset\CommonAssetController -> Glpi\Controller\CommonIndexController (or maybe GenericIndexController)
  • /asset/{type}/search -> /{type}
  • getAssetClass() -> getClass()

@Pierstoval
Copy link
Collaborator Author

Pierstoval commented Oct 14, 2024

Instead of "index" I suggest using the term "Model", because that's both self-explanatory and generic, wdyt?

We would then have "Model list", and "Model form", for routes and controllers naming

@cedric-anne
Copy link
Member

Instead of "index" I suggest using the term "Model", because that's both self-explanatory and generic, wdyt?

We would then have "Model list", and "Model form", for routes and controllers naming

Model is already used from the functional point of view in GLPI. It refers to the hardware models. I think it would be confusing to use it here.

GenericList and GenericForm could be enough.

@Pierstoval Pierstoval force-pushed the generic-controllers branch 5 times, most recently from 8995174 to 5915e5d Compare October 14, 2024 09:39
src/CommonGLPI.php Outdated Show resolved Hide resolved
@Pierstoval Pierstoval force-pushed the generic-controllers branch 3 times, most recently from 0c4185e to c581873 Compare October 15, 2024 14:30
@Pierstoval
Copy link
Collaborator Author

For now, forms are not handled because they're much more complex.

Almost all standard cases are handled by this PR 👌

@Pierstoval Pierstoval marked this pull request as ready for review October 15, 2024 16:01
@Pierstoval Pierstoval changed the title First draft of generic asset controller Generic asset/model/entity controller Oct 15, 2024
Comment on lines +65 to +68
public static function getSectorizedDetails(): array
{
return ["admin", "group"];
}
Copy link
Member

Choose a reason for hiding this comment

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

This one does not work as expected. Indeed, as it is a CommonDropdown class, it is mapped with the DropdownController and the top menu/breadcrumbs are not correctly displayed.

The DropdownController is using the CommonDropdown::displayCentralHeader() that bases the header "sectorized details" based on the $first_level_menu, $second_level_menu and $third_level_menu properties.

We could completely remove the DropdownController usage by moving these properties into the getSectorizedDetails() method and applying the following patch:

diff --git a/src/Glpi/Http/LegacyDropdownRouteListener.php b/src/Glpi/Http/LegacyDropdownRouteListener.php
index 0c8e3ffe8e..3346d98240 100644
--- a/src/Glpi/Http/LegacyDropdownRouteListener.php
+++ b/src/Glpi/Http/LegacyDropdownRouteListener.php
@@ -37,8 +37,8 @@ namespace Glpi\Http;
 use Glpi\Asset\AssetDefinition;
 use Glpi\Asset\AssetModel;
 use Glpi\Asset\AssetType;
-use Glpi\Controller\DropdownController;
 use Glpi\Controller\DropdownFormController;
+use Glpi\Controller\GenericListController;
 use Glpi\Dropdown\Dropdown;
 use Glpi\Dropdown\DropdownDefinition;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
@@ -73,8 +73,13 @@ final readonly class LegacyDropdownRouteListener implements EventSubscriberInter
         if ($class = $this->findDropdownClass($request)) {
             $is_form = \str_ends_with($request->getPathInfo(), '.form.php');
 
-            $request->attributes->set('_controller', $is_form ? DropdownFormController::class : DropdownController::class);
-            $request->attributes->set('class', $class);
+            if ($is_form) {
+                $request->attributes->set('_controller', DropdownFormController::class);
+                $request->attributes->set('class', $class);
+            } else {
+                $request->attributes->set('_controller', GenericListController::class);
+                $request->attributes->set('type', $class);
+            }
         }
     }
 

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Apparently I can even do better: centralize everything in the existing listener and widening it to CommonDBTM instead of just CommonDropdown, and just add an "if" statement to distinguish between DropdownController and GenericListController.

Later on, in another PR, we might "merge" dropdown and generic list controllers

Copy link
Member

Choose a reason for hiding this comment

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

DropdownController does the exact same as GenericListController, except that it displays the header using a different method.

Copy link
Member

Choose a reason for hiding this comment

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

Please remove this file, it is out of the scope of this PR.

Comment on lines +48 to +50
$type = $request->attributes->getString('type');

$class = $this->getClassFromType($type);
Copy link
Member

Choose a reason for hiding this comment

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

IMHO, the type attribute should correspond to the real class name (at least for now).

The normalizeClass logic is already done on the listener side and is not required here.

use Symfony\Component\HttpKernel\Event\RequestEvent;
use Symfony\Component\HttpKernel\KernelEvents;

final readonly class LegacyGenericListRouteListener implements EventSubscriberInterface
Copy link
Member

Choose a reason for hiding this comment

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

This class should be tested.

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.

3 participants