Bug #84167
closed
Negation Bug in TYPO3 8.7 GuzzleHttp \TYPO3\CMS\Core\Utility\GeneralUtility::getUrl()
100%
Description
There's a negation bug in \TYPO3\CMS\Core\Utility\GeneralUtility::getUrl() of TYPO3 8.7.10 that was introduced when the code was converted from curl to GuzzleHttp (see #70056).
The code replaced a check for $content === false in the curl code with !empty($content) in the GuzzleHttp code, which has the consequence of all reponses with a non-empty body being handled like an error and as a result missing the http_code and content_type field in the report.
The bug was introduced in revision 68a6da69a401e083dca1a9e7d368560a52ec0f68 check line 2111 in the old and 2055 in the new code.
The attached patch against the master branch should fix the problem and applies just fine to the 8.7 branch with a line offset.
Files
Updated by Felix Buenemann over 6 years ago
- Subject changed from Negation Bug in TYPO3 8.7 GuttzleHttp \TYPO3\CMS\Core\Utility\GeneralUtility::getUrl() to Negation Bug in TYPO3 8.7 GuzzleHttp \TYPO3\CMS\Core\Utility\GeneralUtility::getUrl()
Updated by Felix Buenemann over 6 years ago
- Related to Feature #70056: Use guzzle added
Updated by Mathias Schreiber over 6 years ago
- Status changed from New to Needs Feedback
- Assignee set to Mathias Schreiber
Would you mind pushing a change?
I can do it too, but then all the credit goes to me while you found the issue in the first place
Updated by Felix Buenemann over 6 years ago
Mathias: Do you mean a PR against the TYPO3-CMS/core Github Repo or where should I direct it?
You can also use git apply <patchfile> which should retain the author and commit message (you would be listed as committer, me as the author).
Updated by Mathias Schreiber over 6 years ago
I'll test whether that thing applies on master, too
Updated by Felix Buenemann over 6 years ago
It should, since the patch is against master from a few minutes ago. Offset in 8.7 is 237 lines or so.
Updated by Felix Buenemann over 6 years ago
- Related to Bug #84170: Inconsistent lib value in \TYPO3\CMS\Core\Utility\GeneralUtility::getUrl() since GuzzleHttp switch added
Updated by Mathias Schreiber over 6 years ago
The file does not apply.
I think this is mainly because it's taken from somewhere within the file structure, not from TYPO3's root dir.
In order to rule out any errors on my side:
git am /Volumes/Macintosh\ HD/Users/mathiasschreiber/Downloads/0001-BUGFIX-Negation-bug-in-getUrl-empty-body-check.patch Applying: Negation bug in getUrl empty body check error: Classes/Utility/GeneralUtility.php: does not exist in index Patch failed at 0001 Negation bug in getUrl empty body check The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".
Tried both from the git root as well as from typo3/sysext/core
Updated by Felix Buenemann over 6 years ago
Oh, it was made against https://github.com/TYPO3-CMS/core I guess I got the wrong repo then.
Updated by Mathias Schreiber over 6 years ago
Try this: https://docs.typo3.org/typo3cms/ContributionWorkflowGuide/Setup/Git/Index.html
This takes roughly 5 minutes to walk through and helps in the manner that (in case questions come up) you're involved directly.
Plus it kinda saves me all the hassle right now :D
Updated by Felix Buenemann over 6 years ago
- File deleted (
0001-BUGFIX-Negation-bug-in-getUrl-empty-body-check.patch)
Updated by Felix Buenemann over 6 years ago
- File 0001-BUGFIX-Negation-bug-in-getUrl-empty-body-check.patch 0001-BUGFIX-Negation-bug-in-getUrl-empty-body-check.patch added
I updated the paths in the patch so that it applies against the main TYPO3.CMS git repo.
Updated by Felix Buenemann over 6 years ago
All right I'll look at the guide. I remember using Gerrit once some years ago and I found the interface very confusing to say the least.
Anyways I'll follow the guide and post an update once done.
Updated by Mathias Schreiber over 6 years ago
I can walk you through it via Voice if you like as well.
Hit me up on Slack when you find the time.
Updated by Gerrit Code Review over 6 years ago
- Status changed from Needs Feedback to Under Review
Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/56039
Updated by Mathias Schreiber over 6 years ago
Followup:
I applied the patch on master and added the necessary info to the commit message.
We'll need to change the commit message around a little bit... just follow the review mentioned in this ticket
Updated by Felix Buenemann over 6 years ago
Yeah, I noticed that git am drops all prefixes in brackets, that's counterproductive here. I'll follow the gerrit workflow directly for the next ticket.
Updated by Gerrit Code Review over 6 years ago
Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/56039
Updated by Gerrit Code Review over 6 years ago
Patch set 3 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/56039
Updated by Gerrit Code Review over 6 years ago
Patch set 4 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/56039
Updated by Felix Buenemann over 6 years ago
- Related to Bug #84173: TYPO3 8.7 GuzzleHttp \TYPO3\CMS\Core\Utility\GeneralUtility::getUrl() returns content_type as array instead of string added
Updated by Gerrit Code Review over 6 years ago
Patch set 1 for branch TYPO3_8-7 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/56047
Updated by Felix Buenemann over 6 years ago
- Status changed from Under Review to Resolved
- % Done changed from 0 to 100
Applied in changeset 2bba5cc7f3d1c2a90bad7bb487e33a8ccdca5e86.
Updated by