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

DEP PHP Support in CMS5 #144

Merged
merged 3 commits into from
Jan 18, 2023
Merged

DEP PHP Support in CMS5 #144

merged 3 commits into from
Jan 18, 2023

Conversation

sabina-talipova
Copy link
Collaborator

@sabina-talipova sabina-talipova mentioned this pull request Dec 9, 2022
Closed
3 tasks
emteknetnz
emteknetnz suggested changes Dec 18, 2022
View reviewed changes
Copy link
Contributor

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Requires a CMS 5 version of https://github.com/tractorcow/silverstripe-proxy-db to be available first

@sabina-talipova
Copy link
Collaborator Author

Passed tests in local env.
Requires PR to be merged first

@sabina-talipova sabina-talipova force-pushed the pulls/3/upgrade-cms5 branch 2 times, most recently from 01bf9b6 to 57b5f29 Compare January 16, 2023 01:40
maxime-rainville
maxime-rainville requested changes Jan 16, 2023
View reviewed changes
Copy link
Collaborator

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

There's a unit test failing.

@sabina-talipova sabina-talipova force-pushed the pulls/3/upgrade-cms5 branch 2 times, most recently from 9ada1e5 to 55a580c Compare January 16, 2023 02:28
@sabina-talipova
Copy link
Collaborator Author

sabina-talipova commented Jan 16, 2023 •
edited
Loading

It failed test by the third-party module issue vendor/maximebf/debugbar/src/DebugBar/Bridge/MonologCollector.php. I've fixed it in my local env and module passed tests.
This module doesn't support PHP 8.1, it fails our CI here:
https://github.com/maximebf/php-debugbar/blob/master/src/DebugBar/Bridge/MonologCollector.php#L60-L62

@maxime-rainville
Copy link
Collaborator

Can you clarify how you fixed it no your local?

This looks like an incompatibility between maximebf/debugbar and Monolog: maximebf/php-debugbar#514

@sabina-talipova
Copy link
Collaborator Author

sabina-talipova commented Jan 16, 2023 •
edited
Loading

Can you clarify how you fixed it no your local?

I created new class LeKoala\DebugBar\MonologCollector insilverstripe-debugbar and copied content of DebugBar\Bridge\MonologCollector class, then use new class in LeKoala\DebugBar\DebugBar. PHPUnit tests are passed.
But I did this only for test purpose.

@sabina-talipova sabina-talipova force-pushed the pulls/3/upgrade-cms5 branch 2 times, most recently from d220f0d to 02e6064 Compare January 16, 2023 22:51
@emteknetnz
Copy link
Contributor

@sabina-talipova could you please update this PR to include the changed you made on your local

@sabina-talipova
Copy link
Collaborator Author

@sabina-talipova could you please update this PR to include the changed you made on your local

DONE

emteknetnz
emteknetnz suggested changes Jan 18, 2023
View reviewed changes
.github/workflows/ci.yml Outdated
@@ -14,3 +14,7 @@ jobs:
# Only run cron on the silverstripe account
if: (github.event_name == 'schedule' && github.repository_owner == 'lekoala') || (github.event_name != 'schedule')
uses: silverstripe/gha-ci/.github/workflows/ci.yml@v1
with:
extra_jobs: |
- composer_require_extra: silverstripe/subsites:^3
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is subsites being added?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

DONE

code/Bridge/MonologCollector.php
* $debugbar->addCollector(new MonologCollector($logger));
* </code>
*/
class MonologCollector extends AbstractProcessingHandler implements DataCollectorInterface, Renderable, MessagesAggregateInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we copy pasting this in from https://github.com/maximebf/php-debugbar/blob/master/src/DebugBar/Bridge/MonologCollector.php?

"maximebf/debugbar": "^1.18", which should be PHP 8.1 compatible, is already included in composer.json?

Copy link
Collaborator Author

@sabina-talipova sabina-talipova Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It looks like this method declaration https://github.com/maximebf/php-debugbar/blob/v1.18.1/src/DebugBar/Bridge/MonologCollector.php#L62 is not compatible with version of monolog/monolog (see here) that we use Monolog\Handler\AbstractProcessingHandler::write(Monolog\LogRecord $record): void. It still fail our tests https://github.com/lekoala/silverstripe-debugbar/actions/runs/3934516960/jobs/6729336460#step:12:62.

This issue was already created maximebf/php-debugbar#514

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, probably need to do this then to workout this issue

Could you please put a class docblock explaining the reason why this class exists, and mention that it should be deprecated and stop being used once the maximebf/php-debugbar works with monolog/monolog:^3

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

DONE

@sabina-talipova sabina-talipova force-pushed the pulls/3/upgrade-cms5 branch 2 times, most recently from b8beaa4 to 9abc6bd Compare January 18, 2023 02:47
emteknetnz
emteknetnz suggested changes Jan 18, 2023
View reviewed changes
Copy link
Contributor

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

One small change please

@GuySartorelli GuySartorelli merged commit 96ebe3a into lekoala:3 Jan 18, 2023
@GuySartorelli GuySartorelli deleted the pulls/3/upgrade-cms5 branch January 18, 2023 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxime-rainville maxime-rainville maxime-rainville requested changes

@emteknetnz emteknetnz emteknetnz approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@sabina-talipova @maxime-rainville @emteknetnz @GuySartorelli