#4680. Group alias. #329

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
4 participants
Member

sembrestels commented Jul 17, 2012

Not ready for merge.

I go committing gradually, and you give me feedback.

@sembrestels sembrestels commented on the diff Jul 17, 2012

engine/schema/mysql.sql
@@ -204,8 +204,10 @@ CREATE TABLE `prefix_geocode_cache` (
CREATE TABLE `prefix_groups_entity` (
`guid` bigint(20) unsigned NOT NULL,
`name` text NOT NULL,
+ `groupname` varchar(128) NOT NULL DEFAULT '',
@sembrestels

sembrestels Jul 17, 2012

Member

This is the definition of an username, but in the upgrade is NULL DEFAULT NULL for backwards compatibility. New installations will have a groupname for each group but old installations not yet.

@cash

cash Jul 18, 2012

Contributor

Why can't we do this with metadata?

@sembrestels

sembrestels Jul 18, 2012

Member

Because I want it as username. It is the best way to do groupname unique and fast to get. We'll need it for all URLs involving groups, and get_group_by_groupname must be fast.

@ewinslow

ewinslow Jul 28, 2012

Owner

I'm not a fan of changing DB tables for this either, but understand the argument, certainly, and am sympathetic to it. I'm also glad you made them optional.

Owner

sembrestels commented on c40b53e Jul 18, 2012

This is a lot of boilerplate!

We should move this code and validate_username() code to a common library, maybe entities.php?

Which name should the function have, if it is for groups and users?

Should we deprecate 'registeruser:validate:username' 'all' by something like 'validate:alias', 'user'?

Owner

sembrestels replied Jul 19, 2012

Please, I need feedback here to continue.

@ewinslow ewinslow commented on the diff Jul 28, 2012

engine/classes/ElggGroup.php
@@ -51,6 +53,15 @@ function __construct($guid = null) {
throw new IOException($msg);
}
+ // See if this is a groupname
+ } else if (is_string($guid)) {
+ $group = get_group_by_groupname($guid);
@ewinslow

ewinslow Jul 28, 2012

Owner

I really do despise these global functions...

@ewinslow ewinslow commented on the diff Jul 28, 2012

engine/classes/ElggGroup.php
@@ -386,6 +414,7 @@ public function save() {
public function getExportableValues() {
return array_merge(parent::getExportableValues(), array(
'name',
+ 'groupname',
@ewinslow

ewinslow Jul 28, 2012

Owner

I don't like the idea of this being exposed via an API. Can we just make it "username"?

@ewinslow ewinslow commented on the diff Jul 28, 2012

engine/lib/group.php
+ // Belts and braces
+ // @todo Tidy into main unicode
+ $blacklist2 = '\'/\\"*& ?#%^(){}[]~?<>;|¬`@-+=';
+
+ for ($n = 0; $n < strlen($blacklist2); $n++) {
+ if (strpos($groupname, $blacklist2[$n]) !== false) {
+ $msg = elgg_echo('registration:invalidchars', array($blacklist2[$n], $blacklist2));
+ $msg = htmlentities($msg, ENT_COMPAT, 'UTF-8');
+ throw new RegistrationException($msg);
+ }
+ }
+
+ $result = true;
+ return elgg_trigger_plugin_hook('registergroup:validate:groupname', 'all',
+ array('groupname' => $groupname), $result);
+}
@ewinslow

ewinslow Jul 28, 2012

Owner

This looks like C&P, no?

@mrclay

mrclay Aug 19, 2012

Owner

Disregard my comment... I'll comment in the ticket.

@ewinslow ewinslow commented on the diff Jul 28, 2012

engine/lib/group.php
+ '\x{2000}-\x{200f}' . // various whitespace
+ '\x{2028}-\x{202f}' . // breaks and control chars
+ '\x{3000}' . // ideographic space
+ '\x{e000}-\x{f8ff}' . // private use
+ ']/u';
+
+ if (
+ preg_match($blacklist, $groupname)
+ ) {
+ // @todo error message needs work
+ throw new RegistrationException(elgg_echo('registration:invalidchars'));
+ }
+
+ // Belts and braces
+ // @todo Tidy into main unicode
+ $blacklist2 = '\'/\\"*& ?#%^(){}[]~?<>;|¬`@-+=';
@ewinslow

ewinslow Jul 28, 2012

Owner

Can we move this into a constant on the relevant class?

@ewinslow ewinslow commented on the diff Jul 28, 2012

engine/lib/group.php
@@ -25,6 +25,104 @@ function get_group_entity_as_row($guid) {
}
/**
+ * Get group by groupname
+ *
+ * @param string $groupname The group's groupname
+ *
+ * @return ElggGroup|false Depending on success
@ewinslow

ewinslow Jul 28, 2012

Owner

New functions should not mix return types.

@mrclay

mrclay Sep 4, 2012

Owner

Not sure we can stick to that rule for fetch functions that return a single object. I rather think we should standardize on returning false. Most libraries standardize on null when the object can't be found (e.g. getElementById), but we're already using false almost everywhere.

I suggest this policy:

  • Try to have a single return type, using false-equivalent values like 0, "", and array() as flags.
  • Mix the return type if the user must be able to distinguish between similar values such as 0 and false (e.g. strpos()).
  • Return false when a single object cannot be found under normal circumstances.
  • For functions that should always return an object under normal conditions, throw an exception if the object is unavailable.

@ewinslow ewinslow commented on the diff Jul 28, 2012

engine/lib/group.php
@@ -25,6 +25,104 @@ function get_group_entity_as_row($guid) {
}
/**
+ * Get group by groupname
+ *
+ * @param string $groupname The group's groupname
+ *
+ * @return ElggGroup|false Depending on success
+ * @since 1.9
+ */
+function get_group_by_groupname($groupname) {
@ewinslow

ewinslow Jul 28, 2012

Owner

As I've said elsewhere, not a fan of new global functions. Let's get this put elsewhere.

@mrclay

mrclay Aug 19, 2012

Owner

While I agree about global funcs, this is the most obvious place to put it until we have a plan for major refactoring.

Owner

ewinslow commented Jul 28, 2012

Looking OK so far. Needs a UI of course -- Since group usernames take up the namespace, I feel like only admins should be able to assign them. Plugins can then allow other members to assign usernames if that feature is desired.

Owner

ewinslow commented Jan 14, 2013

Whatever happened to this? Didn't we decide to go a different direction (i.e. "alias" field on entity table, site/subtype/alias triple must be unique)? I have of such a thing now, so I'm interested if there's other work.

This particular PR doesn't look like it's going anywhere, so closing.

ewinslow closed this Jan 14, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment