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

Update footer.inc #702

Merged
merged 5 commits into from
Nov 22, 2022
Merged

Update footer.inc #702

merged 5 commits into from
Nov 22, 2022

Conversation

rongzedong
Copy link
Contributor

manual View Source is a 404 error page.

manual View Source is a 404 error page.
@rongzedong
Copy link
Contributor Author

link to the manual XML file, will be a Better idea.

@kamil-tekiela
Copy link
Member

I'm not sure how this fixes the problem. Could you explain?

@rongzedong
Copy link
Contributor Author

rongzedong commented Sep 20, 2022 via email

@kamil-tekiela
Copy link
Member

Did you by any chance mean to use && instead of ||. I think that would make more sense.

@kamil-tekiela
Copy link
Member

Do you mean you inserted a ternary operator there somewhere? I cannot see it in this PR.

@cmb69
Copy link
Member

cmb69 commented Sep 20, 2022

We should either remove that link completely, or just use the same URL as for the "Submit a pull request" link.

@kamil-tekiela
Copy link
Member

Well, then someone must be wrong; either you or me. @cmb69 Can you see a ternary operator in the code?
Perhaps, you saw the : starting the if block and you confused it with a ternary operator... but that still would not explain the flaw in logic.

@cmb69
Copy link
Member

cmb69 commented Sep 20, 2022

The patch as is makes no sense; if the 5 first bytes equal "manual", the variable is not empty.

@rongzedong
Copy link
Contributor Author

rongzedong commented Sep 20, 2022 via email

@rongzedong
Copy link
Contributor Author

rongzedong commented Sep 20, 2022 via email

@rongzedong
Copy link
Contributor Author

rongzedong commented Sep 20, 2022 via email

@rongzedong
Copy link
Contributor Author

rongzedong commented Sep 20, 2022

i update the code for my idea. @kamil-tekiela

@rongzedong
Copy link
Contributor Author

@cmb69 @mj @kamil-tekiela , i change the code, please help me, review and approval. think you.

@kamil-tekiela
Copy link
Member

Thanks. The code makes more sense now, but still isn't something that we would merge. However, I think cmb69 might be right that the solution you are proposing isn't the ideal solution.

include/footer.inc Outdated Show resolved Hide resolved
@cmb69
Copy link
Member

cmb69 commented Sep 21, 2022

However, I think cmb69 might be right that the solution you are proposing isn't the ideal solution.

I have overlooked that "view source" as is makes sense for those who want to work on web-php (not doc-en). As such, @rongzedong's suggestion makes sense now, although that code should be refactored.

@kamil-tekiela
Copy link
Member

We could have an else condition too. It would link to doc-en.

@rongzedong
Copy link
Contributor Author

rongzedong commented Sep 21, 2022 via email

include/footer.inc Outdated Show resolved Hide resolved
@rongzedong
Copy link
Contributor Author

@cmb69 i find this,:
BENCHMARK ITERATION RESULT IS:
if (substr($key,0,5 == "HTTP_").... - 0,000481s
if (!strncmp($key, 'HTTP_', 5)).... - 0,000405s

strncmp() is 20% faster than substr() :D

@cmb69
Copy link
Member

cmb69 commented Sep 21, 2022

strncmp() is 20% faster than substr() :D

Yes, I know (substr() constructs a temporary string, what is not needed here). Still, that would only be a micro-optimization. The main reason is that we could easily grep for strncmp when we want to replace it with str_starts_with() in the future.

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Thank you! This is fine to be merged.

@rongzedong rongzedong closed this Nov 22, 2022
@rongzedong rongzedong reopened this Nov 22, 2022
@cmb69 cmb69 merged commit e04e1d7 into php:master Nov 22, 2022
@cmb69
Copy link
Member

cmb69 commented Nov 22, 2022

Looks like this has been overlooked. I've merged it now. Thank you!

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.

3 participants