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: PHP warnings triggered by the test suite #302

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alquerci
Copy link
Contributor

@alquerci alquerci commented Jan 1, 2024

The test should fail when a warning PHP error occurred.

Depends on

How to execute tests?

To make test passes, I apply the following dependencies:

Checkout the "bundle": https://github.com/alquerci/symfony1/tree/fixtest-fail-on-php-errors--dev

Then execute:

test/bin/test --dependency-preferences 'lowest highest'

What is done?

  • Set consistent error_reporting to catch from PHP warnings and higher errors.
  • [validator] Fix two kind of warning
  • [mailer] fix the support for swiftmailler 6
  • Update core autoloader, and apply 4 space indents.
    • data/bin/symfony symfony:test --trace --update-autoloader

What about the CI?

I try to put a PHP error_reporting directive, but the execution environment looks like different than docker.

Next steps

  • notice
  • strict
  • deprecated

@alquerci alquerci force-pushed the fixtest-fail-on-php-errors branch from bd28e37 to 2270fae Compare January 1, 2024 09:47
@alquerci alquerci marked this pull request as draft January 1, 2024 09:58
@alquerci alquerci changed the title [WIP] fix(test): fail on php errors [WIP] fix(test): fail on PHP warning Jan 1, 2024
@alquerci alquerci force-pushed the fixtest-fail-on-php-errors branch from 2270fae to 82caad1 Compare January 1, 2024 10:17
@alquerci alquerci marked this pull request as ready for review January 1, 2024 16:31
@alquerci alquerci changed the title [WIP] fix(test): fail on PHP warning fix(test): fail on PHP warning Jan 1, 2024
@alquerci alquerci force-pushed the fixtest-fail-on-php-errors branch 3 times, most recently from 433b083 to 8d6330e Compare January 7, 2024 17:33
@alquerci
Copy link
Contributor Author

alquerci commented Jan 7, 2024

@thePanz hey, I got it tests passes on GitHub action.

Let me know whether this PR has any chance to be merged.

@alquerci alquerci changed the title fix(test): fail on PHP warning fix: PHP warnings triggered by the test suite Jan 8, 2024
@alquerci alquerci force-pushed the fixtest-fail-on-php-errors branch from 8d6330e to e1e02b2 Compare January 19, 2024 23:49

class_exists('Swift');

if (version_compare(Swift::VERSION, '6.0.0') >= 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This approach is a bit like https://github.com/FriendsOfSymfony1/symfony1/pull/237/files but way more elegant.

Question is, if we are able to support swiftmailer 6 including the doctrine spools, do we really need to support swiftmailer 5?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, I guess not.

But it depends, on the ability of applications using swiftmailer 5 to migrate to swiftmailer 6.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This opens a path to add support for symfony/mailer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do like this approach to support the files being loaded by composer.

@@ -8,6 +8,10 @@
* file that was distributed with this source code.
*/

if (!defined('SF_TEST_WITHOUT_COMPOSER')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@thePanz and @connorhu this will bring support for composer autoloading in the tests and a way towards getting rid of the submodules.

@alquerci alquerci force-pushed the fixtest-fail-on-php-errors branch from e1e02b2 to b8b25b3 Compare March 4, 2024 19:42
@mentalstring mentalstring mentioned this pull request Mar 20, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants