Skip to content

Remove trailing comma in function call to restore < PHP 7.3 compatibility#276

Merged
Thomas-Gelf merged 1 commit intoIcinga:masterfrom
frdmn:fix/lt-php7.3-compatibility
Nov 16, 2021
Merged

Remove trailing comma in function call to restore < PHP 7.3 compatibility#276
Thomas-Gelf merged 1 commit intoIcinga:masterfrom
frdmn:fix/lt-php7.3-compatibility

Conversation

@frdmn
Copy link
Copy Markdown
Contributor

@frdmn frdmn commented Nov 16, 2021

When running PHP 7.0.33 (which should be supported by this module) and upgrading this module to 1.2.0 the database daemon doesn't start properly anymore:

icinga@icinga:~# icingacli vspheredb daemon run
notice: [configwatch] DB configuration loaded
notice: [db] sending DB config to child process
error: DB runner is failing: Got invalid NetString data:
Parse error: syntax error, unexpected ')' in /usr/share/icingaweb2/modules/vspheredb/library/Vspheredb/Daemon/DbCleanup.php on line 45

error: [configwatch] Sending DB Config failed: Connection closed
error: Process exited with exit code 255
error: DB runner is failing: Process exited with exit code 255

The line before line number 45 contains a single trailing comma which was introduced in PHP 7.3: https://wiki.php.net/rfc/trailing-comma-function-calls

To restore compatibility with PHP 5.6 - 7.2, I removed that trailing comma with this PR.

@cla-bot
Copy link
Copy Markdown

cla-bot bot commented Nov 16, 2021

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @frdmn

Details

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please contact us if you think this is the case.

  • If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

@frdmn
Copy link
Copy Markdown
Contributor Author

frdmn commented Nov 16, 2021

Signed

@bobapple
Copy link
Copy Markdown
Member

bobapple commented Nov 16, 2021

@cla-bot check

@cla-bot cla-bot bot added the cla/signed label Nov 16, 2021
@Thomas-Gelf Thomas-Gelf merged commit d5cb1a7 into Icinga:master Nov 16, 2021
@Thomas-Gelf
Copy link
Copy Markdown
Contributor

Thomas-Gelf commented Nov 16, 2021

Thanks for reporting and fixing this! I checked all the other files, that's the only occurrence of this issue.

@frdmn frdmn deleted the fix/lt-php7.3-compatibility branch November 16, 2021 16:38
@lippserd lippserd added this to the v1.2.1 milestone Mar 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

cla/signed

Projects

None yet

Milestone

v1.2.1

Development

Successfully merging this pull request may close these issues.

4 participants

@frdmn @bobapple @Thomas-Gelf @lippserd