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

Fix undefined indexes from user_info overrides. #4834

Merged
merged 5 commits into from Nov 13, 2018

Conversation

Projects
None yet
4 participants
@jdarwood007
Member

jdarwood007 commented Jul 13, 2018

This reverts #4754
Fixes #4288 and #4287
Related to #4819
I wasn't able to reproduce the original issue (https://www.simplemachines.org/community/index.php?topic=559941.0) even after just reverting #4754. I believe #4759 fixed the original issue.

If anyone can give me the full steps to reproduce the errors from these PR/issues after this PR has been applied, let me know.

Signed-off-by: jdarwood007 unmonitored+github@sleepycode.com

Fix undefined indexes from user_info overrides.
This reverts #4754
Fixes #4288 and #4287
I wasn't able to reproduce the original issue (https://www.simplemachines.org/community/index.php?topic=559941.0) even after just reverting #4754.  I believe #4759 fixed the original issue.

If anyone can give me the full steps to reproduce the errors from these PR/issues after this PR has been applied, let me know.

Signed-off-by: jdarwood007 <unmonitored+github@sleepycode.com>

@jdarwood007 jdarwood007 added this to the RC 1 milestone Jul 13, 2018

@albertlast

This pr fix one issue and open the other again,
so i'm not a friend of this.

Fixed updateMemberData to use alert_count instead, as this doesn't re…
…quire us to load up extra information.

Fixed alert_count to actually only show what alerts a user can see as part of the count.  This can cause pagination issues if the user can't see the board the alert is from.

Signed-off-by: jdarwood007 <unmonitored+github@sleepycode.com>
@jdarwood007

This comment has been minimized.

Member

jdarwood007 commented Jul 13, 2018

@albertlast has suggested that the alerts should track id_board as its own column. Which would make more sense and simplify this process.
I think that is a great idea and we can either target that here, or target that in its own PR.

jdarwood007 added some commits Jul 16, 2018

Merge branch 'release-2.1' of github.com:SimpleMachines/SMF2.1 into f…
…etch_alerts

Signed-off-by: jdarwood007 <unmonitored+github@sleepycode.com>

# Conflicts:
#	Sources/Profile-View.php
Minor change
Signed-off-by: jdarwood007 <unmonitored+github@sleepycode.com>
@betonkamil

This comment has been minimized.

betonkamil commented Aug 24, 2018

close if it solves the problem

@Sesquipedalian

With this PR, I found that alerts were not being created at all for mentions. I don't know why.

@albertlast albertlast referenced this pull request Oct 21, 2018

Open

RC1 Checklist #4485

The problem about a lack of alerts for mentions is happening anyway

@Sesquipedalian

This comment has been minimized.

Member

Sesquipedalian commented Oct 25, 2018

Okay, the more I try to untangle exactly what bug or bugs this PR is meant to fix, the most twisted up it seems to get. @jdarwood007, can you tell me exactly what the bug is that this is meant to solve? That should help me know what I am trying to test in order to review this properly.

@jdarwood007

This comment has been minimized.

Member

jdarwood007 commented Oct 26, 2018

Recent commits have removed the undefined indexes.

However I still have concerns over overloading $user_info for handling fetching alerts for another user. Seems the logic is flawed and broken if we have to overload $user_info to just handle fetching alerts.

I don't care whether this gets merged or not. I've stated my concerns, but if we don't want to fix that concern then we can move on.

@Sesquipedalian

This comment has been minimized.

Member

Sesquipedalian commented Oct 26, 2018

Okay, so at this point this PR is best understood as giving an alternative to overloading $user_info, because that is potentially dangerous, correct? If so, then I agree that it is better not to overload $user_info for the reasons you've explained in the past, and I will test this PR based on that understanding.

$val = $val . ' END';
$type = 'raw';
}
else
{
$blub = fetch_alerts($members, false, 0, array(), false);
$val = alert_count($members, false);
$val = count($blub);

This comment has been minimized.

@albertlast

albertlast Oct 26, 2018

Collaborator

Should be not this line deleted?

This comment has been minimized.

@jdarwood007

jdarwood007 Oct 26, 2018

Member

Yes it should.

// Leave a place holder here for now.
// MySQL uses JSON_CONTAINS() to find data, Postgresql seems to be able to natively use it with JSONB column type.
// Maybe a $smcFunc['db_json_supported']?
if (false)

This comment has been minimized.

@albertlast

albertlast Oct 26, 2018

Collaborator

How would look the json approached?

This comment has been minimized.

@jdarwood007

jdarwood007 Oct 26, 2018

Member

It has been so long, I honestly am not 100% sure, but the notes say that postgres and mysql handled json differently.

I don't think its a case for something in 2.1 at this point. Maybe a future version we can identify a way to natively handle json string searches.

@Sesquipedalian Sesquipedalian merged commit 49c9bb3 into SimpleMachines:release-2.1 Nov 13, 2018

2 checks passed

Scrutinizer Analysis: 3 new issues, 1 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment