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

/Sources/ShowAttachments.php ~ compatibility fix #3382

Closed
wants to merge 10 commits into from
Closed

/Sources/ShowAttachments.php ~ compatibility fix #3382

wants to merge 10 commits into from

Conversation

Underdog-01
Copy link
Contributor

  • SMF 2.0.X branch did not use a systematic file extension on attachments (it was void of any extensions)
  • this edit will allow for any generic extension to be removed when said file does not exist in case someone decides to change the behaviour yet again

Signed-off-by: -Underdog- github.underdog@gmail.com

- SMF 2.0.X branch did not use a sytematic file extension on attachments (it was void of any extensions)
- this edit will allow for any generic extension to be removed when said file does not exist in case someone decides to change the behaviour yet again

Signed-off-by: -Underdog- github.underdog@gmail.com
- fixes specific usage of the array which was flagging errors for guests

Signed-off-by: -Underdog- github.underdog@gmail.com
@@ -110,6 +110,7 @@ function showAttachment()
$smcFunc['db_free_result']($request);

$file['filePath'] = getAttachmentFilename($file['filename'], $attachId, $file['id_folder'], false, $file['file_hash']);
$file['filePath'] = !file_exists($file['filePath']) ? substr($file['filePath'], 0, -(strlen(pathinfo($file['filePath'])['extension'])+1)) : $file['filePath'];
Copy link
Contributor

Choose a reason for hiding this comment

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

pathinfo($file['filePath'])['extension']

This will generate a parse error on PHP < 5.5.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot about that change to PHP.
I'm glad they changed it because 2 lines of code to do one small task seems superfluous.

- PHP < 5.5 can not array dereference an array literal


Signed-off-by: -Underdog- github.underdog@gmail.com
@Underdog-01
Copy link
Contributor Author

The attachments issue was a correct fix but the guest $user_info issue was not.
I will have to look for the source of that array error and further test a fix.
The one edit will be removed so that this may be pushed (if deemed adequate).
I see several other issues now anyway and will just pull request a new batch later.

- not the source of the issue seen.  New pull request to be posted later.

Signed-off-by: -Underdog- github.underdog@gmail.com
- array_intersect does not allow simple array vs multidimensional array comparison
- array to string conversion error was contantly showing in log prior to this edit 

Signed-off-by: -Underdog- github.underdog@gmail.com
@Underdog-01
Copy link
Contributor Author

Btw - these issues were apparent when testing an upgrade from the SMF 2.0.X branch.
This is the scenario most of the user base will be applying.

@MissAllSunday
Copy link
Contributor

Wouldn't it be easier if you just cast the var as an array?

$permission = (array) $permission;

since permissionCallback() is not going to be used anywhere else might as well use a lambda

- reverted 1 change

Signed-off-by: -Underdog- github.underdog@gmail.com
- changed standard closure to lambda as per request

Signed-off-by: -Underdog- github.underdog@gmail.com
@Underdog-01
Copy link
Contributor Author

I used gettype because of something I read regarding using is_array with a single empty key of zero not flagging true but I can't seem to find the reference. Even though when I tested this last I had no issue with that scenario.
Anyhow it has been reverted to its previous state and the closure changed to a lambda.

@MissAllSunday
Copy link
Contributor

Dunno why you reverted it, I just said it would be better to just cast the var as an array and avoid doing checks altogether.

@Underdog-01
Copy link
Contributor Author

That changes the array to have an initial key of zero.
When using it in the comparison it might cause an issue to create yet another tier (without adjusting the callback).
This is why I did not do that in the first place.

ie.
$a = array('this' => 'yes', 'that' => 'no', 'other' => 'maybe');
print_r($a);
echo '<br /><br />';
$a = array($a);
print_r($a);

Output is:

Array ( [this] => yes [that] => no [other] => maybe )

Array ( [0] => Array ( [this] => yes [that] => no [other] => maybe ) )

@MissAllSunday
Copy link
Contributor

Please do note that there's a difference between

$var = (array) $var;

That's casting as an array.

And this

$var = array($var) ;

That's adding the var as a value in another new array.

Casting an array as an array does nothing to the array, which is why I suggested it. It only modifies the var if the var is not an array.

Underdog notifications@github.com wrote:

Thatchangesthearraytohaveaninitialkeyofzero.Whenusingitinthecomparisonitmightcauseanissuetocreateyetanothertier(withoutadjustingthecallback).ThisiswhyIdidnotdothatinthefirstplace.Youarereceivingthisbecauseyoucommented.ReplytothisemaildirectlyorviewitonGitHub

- ensure $permission is an array prior its comparison

Signed-off-by: -Underdog- github.underdog@gmail.com
- not tabbed properly

Signed-off-by: -Underdog- github.underdog@gmail.com
@Underdog-01
Copy link
Contributor Author

Oops.. my mistake.
You're correct and it has been changed.

Imo I'm a bit rusty as of late and this is the most revised PR I've ever entered.
My apologies you gals/guys.

@Underdog-01
Copy link
Contributor Author

re. more regarding behaviour of SMF 2.0.X -> SMF 2.1.X
Some avatars are saved as attachments. When viewing other user avatars of that nature with the attachment;avatar URI it says file not found because of the mysql query in the showAttachment() function (missing topic/board id's). I can write a fix for that as well or is this the intended behaviour?

@MissAllSunday
Copy link
Contributor

Avatars shouldn't be saved as attachments anymore. The upgrade is supposed to convert any existing avatars as normal images.

Al so check this PR #3377 it has some changes to the way showAttachment() work

@Underdog-01
Copy link
Contributor Author

I figured as much.
Thanks.

@MissAllSunday
Copy link
Contributor

With the recent additions this PR now has merge conflcits, do you want to resolve them or create a brand new PR based on the recently updated files?

- adjusted file to resolve conflicts with other recent changes

Signed-off-by: -Underdog- github.underdog@gmail.com
@Underdog-01
Copy link
Contributor Author

I tried to resolve the conflict but there must be more edits to the files therefore I will do another p/r as you suggested.

@Underdog-01
Copy link
Contributor Author

Underdog-01 commented Apr 22, 2016

closed

new p/l: #3402

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.

None yet

3 participants