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

Points hooks with long class names don't get saved #83

Closed
JDGrimes opened this Issue Jun 24, 2014 · 7 comments

Comments

Projects
None yet
1 participant
@JDGrimes
Copy link
Member

JDGrimes commented Jun 24, 2014

When the class name for a points hook is too long, it's settings won't be saved. This is caused by the option name being too long. The limit for the length of option names is presently 64 characters (but see https://core.trac.wordpress.org/ticket/13310).

There are two things I'd like to do here. First, we'll have to truncate option names that are too long (or, an alternative would be to MD5 hash the end of the option if it is too long). Secondly, I think we should simplify the points hook ID bases. This will make them shorter, which will make this less of a problem.

@JDGrimes JDGrimes added the bug label Jun 24, 2014

@JDGrimes JDGrimes added this to the 1.5.0 milestone Jun 24, 2014

@JDGrimes

This comment has been minimized.

Copy link
Member

JDGrimes commented Jun 24, 2014

Perhaps instead of making a database change, we should keep the option names based on the class name instead of the simplified ID bases. Otherwise we'll need to run through all of the points hooks on update and resave the options in the new format.

Also, I think the solution with the MD5 hashing is better that just truncating the options. Otherwise we could easily run into conflicts between different points hooks. Another option would be to use the simplified ID base only for options that are too long, although I'd like to avoid introducing this sort of inconsistency.

@JDGrimes

This comment has been minimized.

Copy link
Member

JDGrimes commented Jun 24, 2014

Another possibility: if someone tries to use a class name that is too long, we just tell them that they are _doing_it_wrong(). That may be the best thing to do for 1.5.0. We could punt the rest until later, which would also give us a chance to see what happens on ticket 13310.

@JDGrimes

This comment has been minimized.

Copy link
Member

JDGrimes commented Jun 27, 2014

Punting the rest to 1.6.0.

@JDGrimes JDGrimes modified the milestones: 1.6.0, 1.5.0 Jun 27, 2014

@JDGrimes

This comment has been minimized.

Copy link
Member

JDGrimes commented Aug 15, 2014

Let's punt this again. WP13310 is marked for 4.1-early, so maybe that will go before this is an issue.

@JDGrimes JDGrimes removed this from the 1.6.0 milestone Aug 15, 2014

@JDGrimes JDGrimes modified the milestone: 1.8.0 Nov 5, 2014

@JDGrimes

This comment has been minimized.

Copy link
Member

JDGrimes commented Nov 24, 2014

Closing this for now. We may revisit it in the future.

@JDGrimes JDGrimes closed this Nov 24, 2014

JDGrimes added a commit to WordPoints/woocommerce that referenced this issue Jan 8, 2015

@JDGrimes JDGrimes removed this from the 1.8.0 milestone Jun 27, 2016

@JDGrimes JDGrimes reopened this Jun 27, 2016

@JDGrimes JDGrimes added this to the 2.1.0 milestone Jun 27, 2016

@JDGrimes

This comment has been minimized.

Copy link
Member

JDGrimes commented Jun 27, 2016

Since 4.4, the maximum length of an option is now 191. Now that we are setting 4.4 to our minimum version (see #383), we can safely update the warning to only apply to class names with a length greater than 191. (Which really isn't that necessary, but we'll go ahead and add it anyway, since we already give the notice here.)

@JDGrimes

This comment has been minimized.

Copy link
Member

JDGrimes commented Jun 27, 2016

Note also that the old points hooks are going to be deprecated eventually anyway, see #321.

@JDGrimes JDGrimes self-assigned this Jul 8, 2016

@JDGrimes JDGrimes closed this in 85f8682 Jul 9, 2016

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