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.

Sign up for GitHub

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

Jump to bottom

Update pear/log to 1.14.4 #30484

Merged
merged 4 commits into from
Jun 20, 2024
Merged

Update pear/log to 1.14.4 #30484

merged 4 commits into from
Jun 20, 2024

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jun 18, 2024 •
edited
Loading

Overview

Attempt to resurrect #29235

Before

pear version 1.13.3

After

pear 1.14.3

Technical Details

This was attempted on the first PR & some upstream update was done - but this kinda got a bit messed up on the other PR & I think it failed tests so lessee

I removed our patch on this as it created duplicate copies of the functions - but not sure if there is stuff in it that is still needed - will check in with @demeritcowboy once tests pass

UPDATE seeing 2 issues locally

  • deprecations

PHP Warning: Using strftime-style formatting is deprecated in /srv/civi-sites/dmaster/web/sites/all/modules/civicrm/vendor/pear/log/Log.php on line 831
PHP Warning: Using strftime-style formatting is deprecated in /srv/civi-sites/dmaster/web/sites/all/modules/civicrm/vendor/pear/log/Log.php on line 831
PHP Warning: Using strftime-style formatting is deprecate

-error
Failure in api call for System check: Undefined property: Log_file::$_filename
#0 /srv/civi-sites/dmaster/web/sites/all/modules/civicrm/CRM/Utils/Check/Component/Security.php(63): PHPUnit\Util\ErrorHandler->__invoke()
#1 /srv/civi-sites/dmaster/web/sites/all/modules/civicrm/CRM/Utils/Check/Component.php(76):

Comments

@civibot civibot
Copy link

civibot bot commented Jun 18, 2024

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@civibot civibot bot added the master label Jun 18, 2024
@eileenmcnaughton eileenmcnaughton mentioned this pull request Jun 18, 2024
Closed
@eileenmcnaughton eileenmcnaughton force-pushed the pear branch 2 times, most recently from bb8dbd2 to 5da39e7 Compare June 18, 2024 14:59
@eileenmcnaughton
Copy link
Contributor Author

Test fails are in Using strftime-style formatting is deprecated' contains "Error in call to Pledge_get : API permission check failed for Pledge/get call; insufficient permission: require access CiviCRM and access CiviPledge". - presumably that was in our patch that I didn't apply

@seamuslee001
Copy link
Contributor

@eileenmcnaughton can you remove the changes to composer.lock and re-run your composer update doing composer update pear/log as it seems there are other package updates in this PR

This was referenced Jun 19, 2024
Closed
Merged
Closed
@eileenmcnaughton eileenmcnaughton changed the title Update pear/log to 1.14.3 Update pear/log to 1.14.4 Jun 19, 2024
@eileenmcnaughton eileenmcnaughton force-pushed the pear branch 4 times, most recently from fcfa99e to ae9d03d Compare June 19, 2024 15:40
@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 @demeritcowboy this is passing now - but I have not unravelled the details of the patch that I have stopped applying. At a high level I think this was the intent of the discussion from the PR #29235 which I have tried to rescue here - ie I think the patches were compatibility patches & that @demeritcowboy had done enough of a job upstreaming that we no longer need them

@demeritcowboy
Copy link
Contributor

They were mostly about keeping things that already exist elsewhere working I think. For example the call to setLocale doesn't mean anything for ISO format, but is needed if you create a log with a locale-dependent format string.

I'm not sure off the top of my head which other library packages use pear/log, or if core is the only one. composer might know...

@demeritcowboy
Copy link
Contributor

demeritcowboy commented Jun 19, 2024 •
edited
Loading

composer.lock suggests this is the only use of it, and a universe search for a few variations doesn't show much.

So I think this just needs a run with some cmses. And I don't trust the tests fully for composer.json changes because there are some things that happen before the PR is applied where it's still using the old version.

demeritcowboy
demeritcowboy reviewed Jun 20, 2024
View reviewed changes
tests/phpunit/CRM/Core/ErrorTest.php
@@ -106,8 +106,7 @@ public function testDebugLoggerFormat(): void {
$log->log('Mary had a little lamb');
$log->log('Little lamb');
$config = CRM_Core_Config::singleton();
$fileContents = file_get_contents($log->_filename);
$this->assertEquals($config->configAndLogDir . 'CiviCRM.' . CIVICRM_DOMAIN_ID . '.' . 'my-test.' . CRM_Core_Error::generateLogFileHash($config) . '.log', $log->_filename);
$fileContents = file_get_contents($config->configAndLogDir . 'CiviCRM.' . CIVICRM_DOMAIN_ID . '.' . 'my-test.' . CRM_Core_Error::generateLogFileHash($config) . '.log');
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see they've prevented getting the log file name now, just it means two things:

  • The update to 1.14 will break the logviewer extension. FYI @adixon
  • While maybe not the most important thing in the world, the naming of the file was a relevant thing to test. And especially now if we have to know how it's constructed in order to find it, then that structure should be enforced by a test.

Does our function CRM_Core_Error::generateLogFileName() need to be protected? Could we expose that? The hash function is already public. The Civi::static is public but people shouldn't rely on that.

@demeritcowboy
Copy link
Contributor

Some more info about the patch. Short version is I don't see a need for it, and so also a followup could remove the references to setLocale().

  • There were a couple things happening at the same time:
    • php 8 deprecating strftime
    • the 5.65 security release causing more people to update including php 8 updates and so hitting strftime
    • intl making a change in 8.1.25 that treated the most common default unix locale ("C") as invalid
    • in 5.67 changing the default log file timestamp to be ISO, which appeared to fix it, but only because that meant it no longer called intl
    • at the time it wasn't clear which direction pear/log was going to take to handle strftime

So,

  • pear/log has decided to remove any need for locale-dependency or strftime, so that solves 1,2,3,5.
  • Civi now uses ISO time across all versions that we need to think about, and there don't appear to be any other uses of pear/log.

@eileenmcnaughton
Copy link
Contributor Author

Thanks for all your digging @demeritcowboy - I don't really have my head around it all - I was mostly just trying to get the stalled PR over the line since it seemed really close (it was the first 90% there - leaving the last 10% that takes the other 90% of the time)

@demeritcowboy
Copy link
Contributor

Ok thanks. I'll just run one more test and then I'd say we can merge this and then deal with the filename problem.

@demeritcowboy demeritcowboy merged commit 7fbe8d9 into civicrm:master Jun 20, 2024
1 check passed
@eileenmcnaughton eileenmcnaughton deleted the pear branch June 20, 2024 20:04
@eileenmcnaughton
Copy link
Contributor Author

Nice @demeritcowboy !!

This was referenced Jun 20, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@demeritcowboy demeritcowboy demeritcowboy left review comments

Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eileenmcnaughton @seamuslee001 @demeritcowboy