Skip to content

Add -z flag in phpdbg output (#9669) #9713

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

Closed
wants to merge 1 commit into from
Closed

Conversation

adsr
Copy link
Contributor

@adsr adsr commented Oct 11, 2022

Flag is already present in man page.

(Requesting hacktoberfest-accepted label for Hacktoberfest cred.)

@nielsdos
Copy link
Member

nielsdos commented Mar 20, 2023

I don't know whether you're still interested in getting this merged, but:
It mostly looks good, except on Windows: there the PHP_SHLIB_SUFFIX is "dll", which is one character longer than "so". So there the help overview line for -z will have a space too many which unaligns the column. You could fix this by defining an additional macro in phpdbg_help.c based on PHP_SHLIB_SUFFIX to take into account the spacing.

Edit: ah and it's even different for macos, you might want to check how other SAPIs do this.

@adsr
Copy link
Contributor Author

adsr commented Apr 17, 2023

Removed lib extension to ensure consistent formatting. Alternatives are too much complexity for too little benefit IMO.

@nielsdos
Copy link
Member

nielsdos commented Apr 17, 2023

Alternatives are too much complexity for too little benefit IMO.

I agree.

php --help gives:
-z <file> Load Zend extension <file>.

Perhaps it could be made consistent to that. Either I can do that or you can change the commit. Let me know :)

EDIT: actually this looks fine because it's talking about an example, I'm merging.

@nielsdos nielsdos closed this in 119b062 Jul 7, 2023
@nielsdos
Copy link
Member

nielsdos commented Jul 7, 2023

Merged into 8.1 and above, thanks!

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.

3 participants