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

phpstan lvl 1 #37

Closed
wants to merge 8 commits into from
Closed

phpstan lvl 1 #37

wants to merge 8 commits into from

Conversation

trasher
Copy link
Contributor

@trasher trasher commented Nov 3, 2023

No description provided.

@trasher trasher force-pushed the feature/phpstan branch 2 times, most recently from 8e30231 to 329c3e2 Compare November 3, 2023 07:32
@trasher
Copy link
Contributor Author

trasher commented Nov 3, 2023

No idea why cd is not found, I cannot make phpstan checks work.

@@ -5,6 +5,7 @@
"require-dev": {
"glpi-project/tools": "^0.6",
"php-parallel-lint/php-parallel-lint": "^1.3",
"phpstan/phpstan": "^1.10",
Copy link
Contributor

Choose a reason for hiding this comment

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

You should also add phpstan/extension-installer and phpstan/phpstan-deprecation-rules, to be able to trigger errors when a deprecated method is used.

phpstan.neon Show resolved Hide resolved
.github/workflows/continuous-integration.yml Outdated Show resolved Hide resolved
@@ -308,10 +308,11 @@ public static function updateMailCollectorOnAuthorizationCallback(
'login' => $authorization->fields['email'],
]
);
Html::redirect($mailcollector->getLinkURL());
Copy link
Contributor

Choose a reason for hiding this comment

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

We should try to redirect to $mailcollector->getLinkURL() even if $success === false, and if we do not have a valid mailcollector ID (this is not an expected usecase), we should redirect to the mail collector list. Indeed Html::back() will probably redirect user on the last page of the external provider OAuth authorization page, and is may result in a redirection loop.

        $mailcollector = new MailCollector();
        $mailcollector_id = $params[$mailcollector->getForeignKeyField()] ?? null;
        if ($mailcollector_id !== null && $mailcollector->getFromDB($mailcollector_id)) {
            if ($success) {
                // Store authorized email into MailCollector
                $mailcollector->update(
                    [
                        'id'    => $mailcollector_id,
                        'login' => $authorization->fields['email'],
                    ]
                );
            }
            Html::redirect($mailcollector->getLinkURL());
        }

        Html::back($mailcollector->getSearchURL());

Copy link
Contributor Author

@trasher trasher Nov 3, 2023

Choose a reason for hiding this comment

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

I'm pretty sure getLinkURL will not work as expected if getFromDB fails; see 0fd143c

@trasher
Copy link
Contributor Author

trasher commented Nov 3, 2023

Currently failing with Stub file /var/glpi/stubs/glpi_constants.php does not exist.; but it should exists, path seems corrrect

@cedric-anne
Copy link
Contributor

Currently failing with Stub file /var/glpi/stubs/glpi_constants.php does not exist.; but it should exists, path seems corrrect

The current CI process uses the nightly build of GLPI, and this nightly build does not include the "development" files, like stubs, tests, ...

I will try to produce soon a "plugins CI" image that contains a "dev" version of GLPI. It would probably help to speed up CI on all plugins, and simplify the way GLPI is retrieved.

@trasher
Copy link
Contributor Author

trasher commented Nov 3, 2023

OK, so I'll let this one as draft; and I think I'll stop working on other plugins CI because its going to change.

@trasher trasher closed this Nov 6, 2023
@trasher trasher deleted the feature/phpstan branch November 6, 2023 06:25
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.

2 participants