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

dev/drupal#176 - Allow wider range of psr-log versions to be installed #23409

Merged
merged 1 commit into from
May 11, 2022

Conversation

demeritcowboy
Copy link
Contributor

@demeritcowboy demeritcowboy commented May 9, 2022

Overview

https://lab.civicrm.org/dev/drupal/-/issues/176

This is an alternate version of (ubuntu 22.04-related) #23297

Before

Only psr-log 1.x allowed

After

More

Technical Details

Note that composer.lock is not updated, so this only changes the actual installed version for Drupal 10 and customized builds. Drupal 9 uses 1.1.4 (or at most ~1.0 if not using core-recommended). Drupal 10 requires 2.0, and it sounds like Ubuntu 22.04 requires 3.0 for the bundled civicrm package.

Comments

@civibot
Copy link

civibot bot commented May 9, 2022

(Standard links)

@civibot civibot bot added the master label May 9, 2022
@demeritcowboy demeritcowboy changed the title allow more log dev/drupal#176 - Allow wider range of psr-log versions to be installed May 9, 2022
@demeritcowboy demeritcowboy mentioned this pull request May 9, 2022
@@ -52,7 +52,7 @@ public static function getMap():array {
* @param string $message
* @param array $context
*/
public function log($level, $message, array $context = []) {
public function log($level, $message, array $context = []): void {
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't it need a similar update for all of these files?

$ grep -r 'extend.*Logger' Civi CRM/ ext api/ tests/
Civi/Api4/Action/System/RotateKey.php:    $logger = new class() extends \Psr\Log\AbstractLogger {
CRM/Core/Error/Log.php:class CRM_Core_Error_Log extends \Psr\Log\AbstractLogger {
CRM/Utils/EchoLogger.php:class CRM_Utils_EchoLogger extends Psr\Log\AbstractLogger implements \Psr\Log\LoggerInterface {
CRM/Utils/SystemLogger.php:class CRM_Utils_SystemLogger extends Psr\Log\AbstractLogger implements \Psr\Log\LoggerInterface {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know those things existed. Civi has so much stuff.

@totten
Copy link
Member

totten commented May 11, 2022

Revision looks good.

Yesterday, I ran the regular test-suite with psr/log v3 by unlocking the dependencies (ie rm composer.lock) (and bumping the metadata in a related package). The basic process for testing was like this:

civibuild download tmp-1-1 --type drupal-clean --patch https://github.com/civicrm/civicrm-core/pull/23409
cd tmp-1-1/web/sites/all/modules/civicrm

composer config platform.php 8.0
rm -rf composer.lock  vendor/
composer install

civibuild reinstall tmp-1-1
civi-test-run -b tmp-1-1 -j /tmp/junit all

All the substantive tests passed. (Formally, there was a failure in CRM_Core_ComposerConfigTest::testHardLocks(), but that just means it ran unlocked.)

I didn't do a full test-run with psr/log v2, but that doesn't worry me since (a) v1+v3 both pass and (b) this signature worked for v1+v2+v3 fine in a previous synthetic experiment.

Going forward, I think we should probably have a test-job which does something like this (ie run all tests, but with unlocked dependencies).

@totten totten merged commit 15348d0 into civicrm:master May 11, 2022
@demeritcowboy demeritcowboy deleted the psrlog branch May 12, 2022 00:04
@demeritcowboy
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants