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

Fix CI #12

Merged
merged 1 commit into from
Sep 23, 2023
Merged

Fix CI #12

merged 1 commit into from
Sep 23, 2023

Conversation

LordSimal
Copy link
Collaborator

Refs: #11

"test": "phpunit",
"stan-setup": "cp composer.json composer.backup && composer require --dev phpstan/phpstan:~1.9.0 psalm/phar:~5.4.0 && mv composer.backup composer.json",
"stan-setup": "phive install",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've introduced the phive toolset which we also use in the core.
See https://github.com/cakephp/cakephp/blob/5.x/.github/CONTRIBUTING.md#test-cases-codesniffer-and-static-analysis to learn more about it

@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered ☂️

$loader = new SchemaLoader();
$loader->loadInternalFile(env('FIXTURE_SCHEMA_METADATA'), 'test');
}
require $root . '/vendor/cakephp/cakephp/tests/bootstrap.php';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If this plugin doesn't need a custom test bootstrapping process, then just requiring the core one is fine

@@ -313,7 +315,7 @@ public function copyToWebroot(?string $filename = null, string $path = 'files',

$prefix = null;
if (empty($config['no_prefix'])) {
$prefix = Strings::substring(md5($this->modified->toDateTimeString()), 0, 3) . '-';
$prefix = Strings::substring(md5($this->modified?->toDateTimeString() ?? ''), 0, 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.

this should be looked into.
As said in another issue modified shouldn't be nullable in the first place.

@@ -122,7 +127,7 @@ public function download(string $id): Response
});

$response = $this->getResponse()
->withType($asset->mimetype)
->withType($asset->mimetype ?? 'jpg')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What if the asset doesn't have a mime type? Currently $asset->mimetype is nullable.

@LordSimal LordSimal requested a review from passchn September 23, 2023 10:46
{
parent::setUp();
if (!defined('PLUGIN_ROOT')) {
include __DIR__ . DS . '..' . DS . 'bootstrap.php';
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't run tests from the phpstorm sidebar without this as the bootstrap script seems not to be run then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a bug in PHPStrom.
You can manually tell PHPStorm to use phpunit.xml.dist like so

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah... now it works :)

Mittel (Bildschirmfoto 2023-09-23 um 21 15 33)

@passchn
Copy link
Collaborator

passchn commented Sep 23, 2023

Thank you so much! Tests work for me locally when running with composer, but not through the phpstorm sidebar.. Should be due to the bootstrap.php not being loaded automatically although it being defined in the phpunit config file.

Also thanl you for fixing types and code style. Regarding the nullable fields: This would require a migration so I wanted to wait until I might release some further db changes..

@passchn passchn merged commit d0fa577 into brandcom:cake5 Sep 23, 2023
7 of 8 checks passed
@passchn
Copy link
Collaborator

passchn commented Sep 23, 2023

Was released in version 0.5.1

@passchn passchn mentioned this pull request Sep 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants