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

[Console] Add check for posix_kill function #54893

Closed
wants to merge 1 commit into from

Conversation

k-sahara
Copy link
Contributor

Q A
Branch? 7.1
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #54892
License MIT

@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 7.1 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@MatTheCat
Copy link
Contributor

MatTheCat commented May 12, 2024

Hello, just some feedback:

  • PHPUnit offers a requires annotation which can automatically skip tests if an extension is missing; this is better than sprinkling calls to function_exists (you can see Symfony already @requires extension pcntl on the test methods you modified).

  • POSIX functions are used by quite a few other test cases, so it would probably make sense to update these as well.

  • You targeted the 7.1 branch, but POSIX functions are present in tests since at least 5.4.

At last, the POSIX extension is supposed to be enabled by default, so I’m not sure core Symfony members would want to add checks for it 🤔

@derrabus derrabus modified the milestones: 7.1, 5.4 May 12, 2024
@carsonbot carsonbot changed the title Adding a check for function_exists [Console] Adding a check for function_exists May 12, 2024
@OskarStark OskarStark changed the title [Console] Adding a check for function_exists [Console] Add check for function_exists May 13, 2024
@OskarStark OskarStark changed the title [Console] Add check for function_exists [Console] Add check for posix_kill function May 13, 2024
@alexandre-daubois
Copy link
Contributor

PHPUnit offers a requires annotation which can automatically skip tests if an extension is missing; this is better than sprinkling calls to function_exists (you can see Symfony already @requires extension pcntl on the test methods you modified).

You can even use @requires function posix_kill at the top of your tests 👍

@nicolas-grekas
Copy link
Member

the POSIX extension is supposed to be enabled by default

This means to me we might not want to merge this PR indeed.
E.g. adding requires function json_encode would also be no-go

@k-sahara k-sahara deleted the fix_54892 branch May 15, 2024 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Console test fails if posix module is not enabled.
6 participants