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

Add a hook to allow plugins to blacklist types from tagging #134

Closed
wants to merge 1 commit into from

Conversation

btry
Copy link
Contributor

@btry btry commented Sep 6, 2022

Formcreator has a class in the hard coded blacklist, and the class name will change within 2023. Some other classes should be added. It is easier for plugin maintenance to let other plugin to manage themselve the blacklist

pluginsGLPI/formcreator#2964

@VladoTTX
Copy link
Contributor

VladoTTX commented Sep 7, 2022

One question here. Is it worth maintaining blacklist?
If you already have white list of allowed item types in $CFG_GLPI['plugin_tag_itemtypes'] ?
I think showForItem should be updated to make tags dropdown shown just for itemtypes where you can actually define a tags eg. those from $CFG_GLPI['plugin_tag_itemtypes'] or update canItemtype() to return true for whitelisted types only..

Currently I noticed that for example in GLPI10 it is displayed on Tasks and other unusual places where it is useless..

@cedric-anne
Copy link
Contributor

@VladoTTX, your are right, it may be better to use an inclusion list, instead of an exclusion list, but it may require some more work.

  1. The best way to handle this would probably to define a plugin_tag_get_itemtypes entry in $PLUGIN_HOOKS.

  2. The base list that contains GLPI types should be reviewed, as it is likely to be incomplete (and should probably be returned by a dedicated method, insteaf of being generated during plugin setup).

We recently worked on this kind of list in fields plugin, it may help. see https://github.com/pluginsGLPI/fields/blob/2064d8a8c5ae71efbb82a6e1f89661475e5e7f12/inc/toolbox.class.php#L206-L354

I will not have time right now to work on this topic, but if someone have time to work on it, I would take time to quickly review the PR. Also, it may help to handle #133 in a more proper way.

@btry
Copy link
Contributor Author

btry commented Sep 9, 2022

Hi

I may submit an other version which changes PluginTagTag::canItemtype().

  • use a blacklist for GLPI core itemtypes (hardcoded as it is currently done)
  • use a whitelist for plugins itemtypes, declared via a hook similar to this PR.

@VladoTTX
Copy link
Contributor

And what about starting with removing PluginTagTag::getBlacklistItemtype() and changing PluginTagTag::canItemtype() from:

tag/inc/tag.class.php

Lines 72 to 75 in 4affdcb

public static function canItemtype($itemtype = '') {
return (!class_exists($itemtype)
|| !in_array($itemtype, self::getBlacklistItemtype()));
}

to whitelist version:

   public static function canItemtype($itemtype = '') {
      global  $CFG_GLPI;

      if(!class_exists($itemtype))
         return false;

      foreach($CFG_GLPI['plugin_tag_itemtypes'] as $section => $itemtypes){
         if(in_array($itemtype,$itemtypes))
            return true;
      }

      return false;
   }

This would remove tags from places where they were not meant to be used (but I am not sure about tags witch are without Associated Item types).
Next step would be to rework it in way to rewrite it to PLUGIN_HOOKS to allow also other plugins to register its classes which are interested in having tags, similar way as with fields plugin.

@btry
Copy link
Contributor Author

btry commented Sep 27, 2022

Hi

This should work.

@cedric-anne
Copy link
Contributor

I started to work on this in #135, but it is far from beiing finished.

@pluginsGLPI pluginsGLPI deleted a comment from TR0LLL Jan 3, 2023
@cedric-anne cedric-anne changed the base branch from develop to master November 8, 2023 13:49
@stonebuzz
Copy link
Contributor

Hi

is it still relevant?

@cedric-anne
Copy link
Contributor

IMHO, if there is an itemtype to exclude, maintainers of other plugins could directly open a PR here to add the class in the blacklisted itemtypes. If we provide a hook and I guess that many people will not be aware of it, and will probably still open issues when there is a new itemtype to exclude, even if we add some kind of documentation for it.

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