Make WordPress Core

Opened 8 years ago

Closed 7 years ago

#39676 closed enhancement (fixed)

Remove restriction of minimum site title length of 4 characters

Reported by: flixos90's profile flixos90 Owned by: flixos90's profile flixos90
Milestone: 4.8 Priority: normal
Severity: normal Version:
Component: Networks and Sites Keywords: has-patch has-unit-tests dev-feedback commit
Focuses: multisite Cc:

Description

Currently wpmu_validate_blog_signup() does not allow specification of sites with titles shorter than 4 characters.
We discussed this restriction while dealing with #37616, and came to the conclusion that it should be removed, since sometimes there are cases where shorter names should be allowed. For setups that need the current requirement, it is easily possible to re-add it by using the wpmu_validate_blog_signup filter (see the full discussion on Slack).

While this change is backward-incompatible for people that rely on that requirement, it's not too critical to remove. We will still need to mention it in a dev-note, including the workaround that re-enables the check.

Unit tests for wpmu_validate_blog_signup() should also be added as part of this change.

As part of the #37616 task, this ticket represents number 16 of that ticket's list.

Attachments (5)

39676.diff (909 bytes) - added by milindmore22 8 years ago.
removed entire clause as we can use the filter wpmu_validate_blog_signup fileter
39676.tests.diff (3.8 KB) - added by flixos90 8 years ago.
39676.2.diff (1.3 KB) - added by flixos90 8 years ago.
39676.3.diff (2.7 KB) - added by flixos90 7 years ago.
39676.4.diff (3.1 KB) - added by flixos90 7 years ago.

Download all attachments as: .zip

Change History (22)

#1 @flixos90
8 years ago

  • Owner set to flixos90
  • Status changed from new to assigned

@milindmore22
8 years ago

removed entire clause as we can use the filter wpmu_validate_blog_signup fileter

#2 @milindmore22
8 years ago

  • Keywords has-patch added; needs-patch removed

@flixos90
8 years ago

#3 @flixos90
8 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

Thanks for the patch @milindmore22!

39676.tests.diff introduces unit tests for wpmu_validate_blog_signup().

#4 @flixos90
8 years ago

In 40294:

Multisite: Provide unit tests for wpmu_validate_blog_signup().

See #39676.

#5 @flixos90
8 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 40295:

Multisite: Remove restriction of minimum site name length in wpmu_validate_blog_signup().

It is sometimes desirable to support shorter site names than 4 characters, therefore that restriction should be removed. It is still possible to manually enforce it by using the wpmu_validate_blog_signup filter.

As a result of this change, another is_super_admin() call gets removed which affects the ongoing efforts of working on a network-wide role system.

Props milindmore22.
Fixes #39676. See #37616.

#6 @johnjamesjacoby
8 years ago

  • Keywords 2nd-opinion added
  • Resolution fixed deleted
  • Status changed from closed to reopened

I agree with this change, but am worried about backwards compatibility, so I'm reopening to have more discussion, and frankly, get this ticket a few more eyes and opinions.

I think core should (at least) have a default filter on wpmu_validate_blog_signup to retain the existing 4-character limit.

If this rolls out as-is (allowing 1 character subdomain creation) every popular sub-domain install with open registration will instantly have a flood of 1/2/3 character subdomain sign-ups, which are sure to collide with popularly used subdomains (cdn., src., dev., etc…)

And since subdomains with fewer than 4 characters were previously prevented by WordPress, it’s unlikely anyone running a subdomain multisite installation would have put additional precautions in place in another layer or system.

#7 @stephdau
8 years ago

+1 on @johnjamesjacoby's suggestion of having a default filter on wpmu_validate_blog_signup retaining the 4 chars minimum, which I think is an elegant compromise.

@flixos90
8 years ago

#8 @flixos90
8 years ago

  • Keywords needs-dev-note removed

39676.2.diff uses the wpmu_validate_blog_signup filter to re-add the behavior in a way that it can easily be unhooked.

What we have to think about is the is_super_admin() check as we should get rid of it in some way (see #37616). We might be able to strip it out, since allowing a super admin to use shorter names should be an edge-case, and it would now be possible via a custom filter (it currently is not included in the patch). If we want to retain a 100% BC, we would need to find another existing or new capability to use instead.

I'm removing the needs-dev-note keyword, as we don't need one here anymore since we don't actually remove the behavior.

This ticket was mentioned in Slack in #core-multisite by barry. View the logs.


8 years ago

#10 @jeremyfelt
8 years ago

It feels a little strange to add a new function just to enforce this as a default filter. What if we applied the filter directly to the character limit? (Ignore the filter name)

$min_char_count = absint( apply_filters( 'minimum_character_count', 4 ) );

if ( strlen( $blogname ) < $min_char_count ) {
    $errors->add('blogname',  __( "Site name must be at least $min_char_count characters." ) );
}

Also - we could just revert most of this and remove the is_super_admin() check. The existing wpmu_validate_blog_signup filter, while somewhat painful, makes it possible to filter this already. It'd sure be nice if those error keys were unique though. :)

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


8 years ago

#12 @flixos90
8 years ago

  • Keywords needs-patch needs-unit-tests added; has-patch has-unit-tests 2nd-opinion removed

Summary of latest discussion (see https://wordpress.slack.com/archives/C03BVB47S/p1491252601769000):

  • Let's partly revert [40295] (everything but the removal of is_super_admin()).
  • Introduce a filter as in @jeremyfelt's above proposal. The name should be specific to sites though (minimum_site_name_length or minimum_blogname_length are possibilities). Two unit tests using that filter (one with a higher restriction, the other with a lower one) should be introduced at the same time.

I think these two steps should be separate commits.

#13 @flixos90
8 years ago

In 40391:

Multisite: Partially revert [40295].

[40295] removed the restriction of a minimum amount of characters for new site names, which could cause unexpected behavior. That changeset is reverted here with the exception of the removal of the is_super_admin() check, which can safely be omitted. A new filter for the minimum site name length will be introduced later to be able to modify that behavior.

See #39676, #37616.

@flixos90
7 years ago

#14 @flixos90
7 years ago

  • Keywords has-patch has-unit-tests dev-feedback added; needs-patch needs-unit-tests removed

39676.3.diff introduces a minimum_site_name_length filter as discussed before. Unit tests are provided as well.

Is minimum_site_name_length the appropriate name for that filter? Should we pass any additional data to it?

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


7 years ago

@flixos90
7 years ago

#16 @flixos90
7 years ago

  • Keywords commit added

Per review by @jeremyfelt and @stephdau, 39676.4.diff fixes an issue with the i18n string in the previous patch, which still had the original limit of 4 hardcoded. Now the string is dynamic, taking the filtered value into account.

#17 @flixos90
7 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 40589:

Multisite: Introduce minimum_site_name_length filter.

Prior to this change, the minimum site name length checked in wpmu_validate_blog_signup() was set to a fixed value of 4. The new filter allows tweaking this value, as there may be cases where shorter site names may be required.

Fixes #39676.

Note: See TracTickets for help on using tickets.