-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Migrate glpi-project/tools to core #22465
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
base: 11.0/bugfixes
Are you sure you want to change the base?
Migrate glpi-project/tools to core #22465
Conversation
b0d64a4 to
4aa5600
Compare
AdrienClairembault
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you'll need to rebase and update the header (2026)
|
|
||
| class CompileTwigTemplatesCommand extends AbstractCommand | ||
| { | ||
| protected const ALLOW_PLUGIN_OPTION = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| protected const ALLOW_PLUGIN_OPTION = 1; | |
| protected const ALLOW_PLUGIN_OPTION = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This make me realize that the tool folder is not linted (at least not by phpstan), maybe we should change that?
Can be done in a separate PR.
| $compile_input = new ArrayInput($options); | ||
| $compile_cmd->run($compile_input, $this->output); | ||
|
|
||
| $this->io->writeln("<question>Extracting translations from files</question>"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem like a question. Maybe it should be info instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| $args['F_ARGS_X'], | ||
| $args['F_ARGS_NX'] | ||
| ); | ||
| system($cmd); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could use symfony/process? I think its already required by GLPI's core.
…no header warning
657aa2b to
5554081
Compare
|
|
||
| locales-compile: ## Compile locales | ||
| @$(PLUGIN) vendor/bin/plugin-release --compile-mo | ||
| @$(CONSOLE) tools:plugin:release --compile-mo --plugin=$(PLUGIN_DIR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, it is not necessary to maintain 2 distinct levels of commands. We will probably never have more than 10 commands in the tools "namespace".
| @$(CONSOLE) tools:plugin:release --compile-mo --plugin=$(PLUGIN_DIR) | |
| @$(CONSOLE) tools:plugin_release --compile-mo --plugin=$(PLUGIN_DIR) |
| "typescript": "^5.9.3", | ||
| "vue": "^3.5.26" | ||
| "vue": "^3.5.26", | ||
| "vue3-gettext": "^4.0.0-beta.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, it should be moved into the dev dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes made to this file should not be necessary once #22568 will be merged.
| /** @var bool Declare the command has supporting plugging. */ | ||
| protected const ALLOW_PLUGIN_OPTION = false; | ||
|
|
||
| /** @var bool Declare the command supporting plugging and require it to be set. */ | ||
| protected const REQUIRE_PLUGIN_OPTION = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is preferable to use methods for this. For instance
abstract protected function isPluginOptionAvailable(): bool;| $this->addOption( | ||
| 'plugin', | ||
| 'p', | ||
| InputOption::VALUE_REQUIRED, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not make the option mandatory, it just make its value required. bin/console tools:command_name and bin/console tools:command_name --plugin=XXX will be valid usages.
| // Propose version | ||
| if ($input->getOption('propose')) { | ||
| $this->proposeVersion(); | ||
| return Command::SUCCESS; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is useless, I think noone uses it. We can remove this feature.
| // Release or Check-Only | ||
| $release_version = $input->getOption('release'); | ||
| $input_commit = $input->getOption('commit'); | ||
| $extra = $input->getOption('extra'); | ||
|
|
||
| if (($extra || $input_commit) && (!$extra || !$input_commit || !$release_version)) { | ||
| $this->io->error('You have to specify --release, --commit and --extra all together'); | ||
| return Command::FAILURE; | ||
| } | ||
|
|
||
| if ($release_version) { | ||
| if (!$input->getOption('dont-check')) { | ||
| if (!$this->validVersion($release_version)) { | ||
| $this->io->error(sprintf('%s is not a valid version number!', $release_version)); | ||
| return Command::FAILURE; | ||
| } | ||
|
|
||
| if (!$this->checkVersion($release_version)) { | ||
| return Command::FAILURE; | ||
| } | ||
| } | ||
|
|
||
| if ($input_commit) { | ||
| if (!$this->validCommit($input_commit)) { | ||
| $this->io->error(sprintf('Invalid commit ref %s', $input_commit)); | ||
| return Command::FAILURE; | ||
| } | ||
| } elseif (!$this->isExistingVersion($release_version)) { | ||
| $this->io->error(sprintf('Tag %s does not exist!', $release_version)); | ||
| return Command::FAILURE; | ||
| } | ||
| } else { | ||
| // If no release version specified, use latest version | ||
| $release_version = $this->getLatestVersion(); | ||
| if (!$release_version) { | ||
| $this->io->error('No tags found in the repository.'); | ||
| return Command::FAILURE; | ||
| } | ||
|
|
||
| if ($input->getOption('force')) { | ||
| // Force mode, proceed directly | ||
| } else { | ||
| if (!$this->io->confirm(sprintf('Do you want to build version %s?', $release_version), true)) { | ||
| return Command::SUCCESS; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, we should only use the version provided by the plugin_xxx_init() function. There is no need to have a such complex way to compute the release version name.
| if ($input->getOption('check-only')) { | ||
| $this->io->note('*** Entering *check-only* mode ***'); | ||
| $this->io->success('Check-only mode finished.'); | ||
| return Command::SUCCESS; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure this is usefull if we simplify the other parts of the script.
| if (!$input->getOption('nogithub')) { | ||
| $this->createGithubRelease($release_version, $input_commit, $tarball); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is useless now. We have automatize it using Github workflows.
| // Sign | ||
| if (!$input->getOption('nosign')) { | ||
| $this->sign($tarball); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trasher Is the plugin release archive GPG sign still used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's never been used on GLPI as far as I remember.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More globally, the python release script has been made for another project of mine; and it's just been quickly adapted to suit GLPI plugins. Many "features" have never been used, and are probably useless.

Checklist before requesting a review
Please delete options that are not relevant.
Description
Migrate https://github.com/glpi-project/tools into the core with also a converted python/bash scripts so all are runned as Symfony Command.
Doing that migration will avoid the dependencies exception libraries could have when they were requiring a different glpi-project/tools
For the future we could put glpi-project/tools as archived but we can't remove it from packagist as plugin supporting prior to GLPI 11.0.5 version would have to still install that library.
It's can be release in 11.0.5 as it was currently only a dev dependencies, so in worth case if the user try to run a command it will just not work.
It will force local dev env to be up to date (or at least be in 11.0.5-dev)
Template https://github.com/pluginsGLPI/empty should also be updated to remove that composer dependency once this PR is merged.
TODOs
PHPStan Rules(already present in core MissingGlobalVarTypeRule)What was done
LicenceHeadersCheckCommand.phpandCompileTwigTemplatesCommand.php(no major edit - namespace and print changes)plugin-releasepython file to PHPextract-localbash file to PHPInterconnected PR (merge order) :
Whatsapp plugin got a dedicated PR to test the modification (see in the feed under for the link)