BUGFIX: FileTypeIconViewHelper should work with new $asset argument #2188
Conversation
|
Hi |
|
Hey @chkoeppel, thanks for your first contribution to this repository! The failed test is not your fault - have a look at the current build status: https://travis-ci.org/neos/neos-development-collection/branches - 4.1 is also broken without your change. Cheers, |
| @@ -75,7 +75,9 @@ public function initializeArguments() | |||
| */ | |||
| public function render(AssetInterface $file = null, AssetInterface $asset = null, string $filename = null, $width = null, $height = null) | |||
| { | |||
| $asset = $file; | |||
| if (!is_null($file)) { | |||
There was a problem hiding this comment.
Mhm with that, if you give both $asset and $file, the deprecated file would take precedence.
Nitpick using $file !== null is more common in the Neos code.
I would recomend:
if($asset === null) {
$asset = $file;
}
There was a problem hiding this comment.
Thank you
- shall I adapt the code to your solution HERE ...
(another commit, no ?)
OR create a new PullRequest ? (squash)
|
Sure, just amend your commit or add another and we’ll squash it while merging. |
aertmann
changed the title
|
Should actually go into 4.0 as that's the lowest branch the bug exists in https://github.com/neos/neos-development-collection/blob/4.0/Neos.Media/Classes/ViewHelpers/FileTypeIconViewHelper.php#L78 |
|
Really SORRY for all these inconvenience ... Should I have closed and reopen a new one ?! |
|
You'll have to reopen with the correct target branch (just reference the new PR from here and close this one). You can't change the target to a lower branch within a PR without this happening afaik, only higher is possible. |
|
@chkoeppel Really don't be sorry, it's a bit of a complicated process when having to maintain multiple versions simultaneously. Happy to guide you through it. You haven't made any mistakes per say, we just need to make sure we target the lowest version with the bug so it can be fixed in all versions it's present in. You have two options, either you can reset your existing branch to be based on 4.0 and then apply the change again and force push it. Alternatively you can create a new branch and open a new pull request. Either is fine, depends on your Git skill level. |
|
If you wanna keep the existing PR and just update your branch, then you can change the base by clicking "Edit" next to the title. |
|
Nvm you already did, then you just need to update your branch to be mergeable with 4.0 with your change applied on top. |
|
@aertmann I saw he is using 4.1 within his fork as base so while he could reset his 4.1 branch to the latest 4.0 commit (and put his commits on top), I think from this point it would be less complicated to just reopen? Maybe on a new bugfix branch based on 4.0? @chkoeppel As Aske already said, no problem at all, we're all very happy to guide you trough the process which can be a little complicated at the beginning. Just ask anything that is unclear for you to proceed. |
|
... just want to be sure what I do now, - I need some minutes of reflection before trying ... |
If the newly added
$assetargument is used, it's ignored and overridden to the deprecated$fileargument.Resolves #2186