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 Subs-Db-mysql.php #4065

Merged
merged 5 commits into from Jun 5, 2017
Merged

Update Subs-Db-mysql.php #4065

merged 5 commits into from Jun 5, 2017

Conversation

Underdog-01
Copy link
Contributor

@Underdog-01 Underdog-01 commented May 23, 2017

  • any modification or future SMF update that includes additions/changes for where clauses contained within the $user_info array (using the query_ prefix) will be included using this method.
  • Imo this should also be applied to the SMF 2.0.15 update

Author: Underdog github.underdog@gmail.com
Date: Mon May 22 23:07:25 2017 -0400

On branch patch-22

Changes to be committed:
modified: Sources/Subs-Db-mysql.php

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

- any modification or future SMF update that includes additions/changes for where clauses contained within the $user_info array (using the query_ prefix) will be included using this method.
- Imo this should also be applied to the SMF 2.0.15 update

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

Author:    Underdog <github.underdog@gmail.com>
Date:      Mon May 22 21:24:25 2017 -0400

On branch patch-22
Your branch is up-to-date with 'origin/patch-22'.

Changes to be committed:
modified:   Sources/Subs-Db-mysql.php
@albertlast
Copy link
Collaborator

Any reason why not the Subs-Db-postgresql is touched?
https://github.com/SimpleMachines/SMF2.1/blob/release-2.1/Sources/Subs-Db-postgresql.php#L131

Copy link
Member

@Sesquipedalian Sesquipedalian left a comment

Choose a reason for hiding this comment

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

I'm not sure that this is a very helpful change. At the very least, strpos() should be used rather than preg_grep(). But moreover, since we already know the only two permitted values, the current method of doing strict equality tests and returning immediately on success is the most efficient approach possible.


if ($matches[1] === 'query_wanna_see_board')
return $user_info['query_wanna_see_board'];
$user_queries = array_values(preg_grep('/^query_/', array_keys($user_info)));
Copy link
Member

Choose a reason for hiding this comment

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

No need to use a regex here. It'd be much more efficient to use strpos() in this case.

@Sesquipedalian
Copy link
Member

Can you give an example, @Underdog-01, of how a mod would want to use this change? What sort of "query_*" WHERE clause placeholder do you imagine a mod would want to add?

@jdarwood007
Copy link
Member

@Sesquipedalian,

I can give an example from SimpleDesk, where we do use a query_see_ticket. To accomplish our needs we use a wrapper around queries that make use of it. Although I myself am not sure of this implantation being the correct method of doing so.

/**
 *	Query wrapper around $smcFunc['db_query'] for helpdesk specific operations.
 *
 *	This function provides a basic wrapper around SMF's internal $smcFunc['db_query'] function, adding the parameter {query_see_ticket}
 *	to it, specifically so that ticket visibility can be enforced in a query without being aware of the specific rules for the user.
 *
 *	If not previously loaded, user permissions are loaded with {@link shd_load_user_perms()}.
 *
 *	@param string $identifier SMF-style query identifier for database backend-specific replacements
 *	@param string $db_string Standard SMF 2.0 style database query, including {query_see_ticket} if appropriate
 *	@param array $db_values Standard SMF 2.0 style hash map of parameters to inject into the query
 *	@param resource $connection A database connection variable, if supplied, to override the one used by SMF by default
 *	@return resource Standard database query resource, suitable for processing with other $smcFunc['db_*'] functions
 *
 *	@see shd_load_user_perms()
 *	@since 1.0
*/
function shd_db_query($identifier, $db_string, $db_values = array(), $connection = null)
{
	global $user_info, $smcFunc;

	$replacements = array(
		'{query_see_ticket}' => $user_info['query_see_ticket'],
	);

	$db_string = str_replace(array_keys($replacements), array_values($replacements), $db_string);
	$db_values['user_info_id'] = $user_info['id'];

	return $smcFunc['db_query']($identifier, $db_string, $db_values, $connection);
}

@Underdog-01
Copy link
Contributor Author

I have had to manually edit popular mods due to the change.
SMF Arcade & SMF Project tools also use it. SP might use it as well but can't remember atm if I had to make an edit for it.
This likely goes for other mods those authors have developed and also those I have not mentioned.
ie. Niko, Sinan, Arantor, etc.

SMF itself uses the $user_info array to pass info for db queries in this fashion for quite some time now so it makes sense to me to allow dynamic usage for mod authors.
I will change it to use strpos but I do so like one liners.
Everyone so concerned about those milliseconds adding up.

Signed-off-by: Underdog-01 <github.underdog@gmail.com>
@albertlast
Copy link
Collaborator

albertlast commented May 24, 2017

I keep missing an awnser of my question,
where keep the support of this for postgres?

- any modification or future SMF update that includes additions/changes for where clauses contained within the $user_info array (using the query_ prefix) will be included using this method.

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

@albertlast: done.

@Sesquipedalian
Copy link
Member

Sesquipedalian commented May 25, 2017

Okay, that explanation makes sense. Just trim off the trailing whitespace after the closing ?>, and this should be good to go, in my opinion.

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

done.
My editor also removed some other rogue white-spaces

Signed-off-by: Underdog-01 <github.underdog@gmail.com>
Copy link
Member

@Sesquipedalian Sesquipedalian left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@colinschoen colinschoen left a comment

Choose a reason for hiding this comment

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

LGTM approval

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants