Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants