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

Log to specific event log file #361

Merged
merged 19 commits into from
May 25, 2021

Conversation

drjayvee
Copy link
Contributor

Q A
Fixed tickets #309 #346

@PabloKowalczyk
Copy link
Collaborator

Hello @drjayvee, thanks for your PR!
Looks like CI is not happy, I saw you just added new code over 2.3.x branch, but you should use #346 as a base branch, then add your code and finally push it here (feel free to use force push, it's your fork anyway).
Sorry for that bureaucracy, but it is needed, hope you know it :)

@PabloKowalczyk PabloKowalczyk added this to the v2.3 milestone Apr 27, 2021
@drjayvee drjayvee force-pushed the log_to_specific_event_log_file branch from 75aee1c to cb1a425 Compare May 6, 2021 13:06
@drjayvee
Copy link
Contributor Author

drjayvee commented May 6, 2021

Whoops, my previous branch actually didn't have @rrushton's changes at all. Must have happened while cherry-picking.

I now merged alisqi@f07569e (from #346) into 2.3.x.

@PabloKowalczyk
Copy link
Collaborator

Thanks, now better :) CI seems to be unhappy, one test fails and CS fixer reports some issues.

@drjayvee
Copy link
Contributor Author

drjayvee commented May 6, 2021

Regarding the PHPUnit test:

There was 1 error:

1) Crunz\Tests\EndToEnd\LoggerTest::test_event_logging_override
Error: Call to undefined method Crunz\Tests\EndToEnd\LoggerTest::assertFileDoesNotExist()

/home/runner/work/crunz/crunz/tests/EndToEnd/LoggerTest.php:60
/home/runner/work/crunz/crunz/vendor/phpunit/phpunit/phpunit:61

This works for me with the PHPUnit installed through composer.

Is this something I need to fix? Using assertTrue(is_file())?

@PabloKowalczyk
Copy link
Collaborator

Method assertFileDoesNotExist should be available in PHPUnit, what is installed version of PHPUnit?

@drjayvee
Copy link
Contributor Author

drjayvee commented May 7, 2021

For me:

../vendor/bin/phpunit --version
PHPUnit 9.5.4 by Sebastian Bergmann and contributors.

But the Code_Checks uses PHPUnit 8.5.15, which does fit composer.json.

@PabloKowalczyk
Copy link
Collaborator

You are right, PHPUnit 8.5 is required on PHP 7.2 and it is also installed on "prefers lowest" jobs.
Could you add assertFileDoesNotExist method to \Crunz\Tests\TestCase\PolyfillAssertTrait with file_exists check? With this code will be forward compatible.

@drjayvee
Copy link
Contributor Author

Hi @PabloKowalczyk, sorry for taking so long again, but here you go!

@PabloKowalczyk
Copy link
Collaborator

Almost there :) One case failed in CI, it's related to CSFixer.

@PabloKowalczyk
Copy link
Collaborator

Sorry, now PHPStan reports some issues.

@drjayvee
Copy link
Contributor Author

There we go. I should have probably tried running those tools locally as best as I can.

@PabloKowalczyk
Copy link
Collaborator

There is composer crunz:cs-fix and composer phpstan:check shortcuts for this, but make sure you have Symfony at v4.4 (same as in CI).
CI still fails, now CSFixer issue.

@drjayvee
Copy link
Contributor Author

Hm,

$ composer crunz:cs-fix
> @php vendor/bin/php-cs-fixer fix --diff -v --ansi
Could not open input file: vendor/bin/php-cs-fixer
Script @php vendor/bin/php-cs-fixer fix --diff -v --ansi handling the crunz:cs-fix event returned with error code 1

Should php-cs-fixer be composer required perhaps?

@PabloKowalczyk PabloKowalczyk merged commit 322a6ea into lavary:2.3.x May 25, 2021
@PabloKowalczyk
Copy link
Collaborator

Should php-cs-fixer be composer required perhaps?

It is, but only in require-dev section.

PR merged, thanks @drjayvee.

@PabloKowalczyk PabloKowalczyk mentioned this pull request May 25, 2021
@drjayvee
Copy link
Contributor Author

It is, but only in require-dev section.

I don't see it!

PR merged, thanks @drjayvee.

Thanks a lot for the fresh release. Also, thanks to @rrushton for the actual code!

This was referenced May 25, 2021
@flippedbits
Copy link

Thanks @drjayvee for seeing this through!

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.

3 participants