As of issue #13, the solution seems to be hashing all deltas as opposed to only those longer than 32 characters. In order to implement this, we need to do two things:

  • Make the change illustrated in #3
  • Write an update hook to update the effected deltas in the block table

We cannot sanitize the delta on input because the bad characters contribute to the uniqueness of the delta and also serve as separators for the different parts of the facet that the delta is associated with. We don't want to sanitize the data on output because there are only limited locations where the bad characters aren't filtered and we would have to write code targeting those specific areas. See #13 for more details.

Original Post

I am in the process of moving to FacetAPI, migrating my old to new blocks. I show my blocks through Context. One block, a multi-value textfield (list<text>), got a strange block CSS ID,

<tr id="facetapi-search_api@flatstore:block:color"
Note the @ sign, which breaks JavaScript in the Context blocks UI.

Other FacetAPI blocks got a hashed ID, "facetapi-23132-Ls9Hv4CHzroOMWtunOOHom3uv", but this color facet block didn't,...

The same thing happens to a few other blocks,
facetapi-search_api@flatstore:block:size

I can't figure this out, what is causing this?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Title: Strange FacetApi block ID breaks Javascript in Context UI » Strange FacetApi block delta's (breaks Javascript in Context UI)

The problem is really the block delta, for example:

/admin/structure/block/manage/facetapi/search_api@flatstore:block:size/configure

A delta "search_api@flatstore:block:size" doesn't seem right :)

The solution would be to force-hash all block IDs. Let's have a look how to do that.

Anonymous’s picture

Title: Strange FacetApi block delta's (breaks Javascript in Context UI) » Facet API generates invalid block delta's like "search_api@flatstore:block:size "
Project: Search API » Facet API
Component: Facets » Code
Priority: Normal » Major

It's an issue with Facet API.

Summary: Facet API's integration with Search API generated block delta's like search_api@flatstore:block:size which breaks jQuery (can't have @ signs in CSS ID's!)

Anonymous’s picture

This seems to be the problem:

function facetapi_hash_delta($delta) {
  return (strlen($delta) <= 32) ? $delta : substr(drupal_hash_base64($delta), 0, 32);
}

Delta's shorter than 32 chars are not hashed, but Search API's delta's have "@" signs in them. So they should always be hashed,... Solution could be:

function facetapi_hash_delta($delta) {
  return substr(drupal_hash_base64($delta), 0, 32);
}

What do you think? Should we force-hash all delta's, or is Search API providing bad delta's?

drunken monkey’s picture

apachesolr's Facet API searchers also have @s in them, so if this really the problem (and I agree it looks like it) than it's a Facet API problem, not a usage problem. In any case, even if implementors were responsible for providing valid JS identifiers as searcher IDs, this would have to be specified.

So yes, always hashing block deltas seems like the best option to me. It also looks more consistent that way. ;) But let's see what Chris says.

Anonymous’s picture

Yeah, you can't use @ in a jQuery selector, $('block-id@search-api') breaks JS. I noticed it in the Context UI blocks system.

cpliakas’s picture

Hmm, I 100% agree that these characters shouldn't be allowed. I thought we fixed that pre-beta1 (guess not!). I will try to confirm this bug and then see why these characters are making their way into the markup.

Thanks for reporting,
Chris

cpliakas’s picture

Status: Active » Postponed (maintainer needs more info)

So after some research, this issue is not present in Apache Solr Search Integration, and Facet API appears to use the drupal_html_id() and drupal_html_css() functions appropriately. There was also an issue closed some time ago at #1150662: Facet blocks not remembering overridden settings which could be related to what is posted here. The block deltas are escaped properly by core, so if you are using the Context module which the title suggests, I would suspect that Context isn't properly escaping the IDs.

Anonymous’s picture

The id's show in the Drupal core block list, and in their URLs, like
/admin/structure/block/manage/facetapi/search_api@flatstore:block:size/configure

Is that normal? If it is, then indeed it's Context UI that does not escape them. Shall I open a issue with them?

cpliakas’s picture

Status: Postponed (maintainer needs more info) » Active
FileSize
431.19 KB

The /admin/structure/block/manage/facetapi/search_api@flatstore:block:size/configure in the URL is normal and works fine. However after looking into the core Drupal block list as you mentioned, the values are not properly escaped for classes and IDs. This is definitely an issue. I would actually say this is a core Drupal issue for not being able to handle this, but I also think that Facet API has to modify its strategy to deal with this. A couple of ideas:

  • Hash everything (as you proposed
  • Find a way to hook into that form and force the values to be sanitized (icky)
  • Anything else?

Thanks for testing,
Chris

Anonymous’s picture

Aha, well for short term we can just hash everything? It has no negative side-effect, I think. And for long term we can file an issue with D8 core. Maybe it will be ported back to D7.

cpliakas’s picture

Yeah, it's ugly but I think it might be the best option here. There is still a nice CSS ID for the unordered list inside of the block, so you shouldn't have to do much with the hash anyways.

Thanks for the suggestion,
Chris

Nick_vh’s picture

Maybe if the problem is context you could take a look at http://drupal.org/project/context_block_class but this is only for D6 unfortunately...

Other then that I think hashing everything makes up for a solid solution indeed so my vote for that. I'm not sure why sanitizing is itchy? Maybe I have a wrong take on that but something like

$new_string = preg_replace(“/[^a-zA-Z0-9]/”, “”, $string);

Could be simple yet effective?

cpliakas’s picture

Well, the way that sanitizing would have to be done is only for that specific page, and we wouldn't want to do it on input for a couple of reasons. First, characters such as "@" and ":" are part of the uniqueness of the name, so stripping them has a small chance of causing collisions. Probably not an issue, but why introduce the possibility of something like that. Second, the deltas contain colons which separate the different parts of the item. For example, "apachesolr@solr:block:bundle" means that the searcher is "apachesolr@solr", "block" is the realm, and "bundle" is the facet. Sanitizing this on input would be problematic because this system would be broken. Sanitizing this on output would have to be targeted to the pages that don't work correctly. I don't want to go down that road either.

So I guess hashing everything is the way to go here. We will need an update function to update the deltas in the block table as well.

cpliakas’s picture

Title: Facet API generates invalid block delta's like "search_api@flatstore:block:size " » Block deltas contain characters that aren't correctly filtered as CSS identifiers on block administration pages

Changing title because the deltas are valid, they just contain characters that are problematic for JS.

cpliakas’s picture

Status: Active » Needs review
FileSize
2.45 KB

The attached patch is my first attempt at fixing the issue. Make sure the blocks that are currently enabled remain enabled. Would love to get some testing before committing.

Thanks,
Chris

Anonymous’s picture

Hi, I applied the patch and ran update.php without problems. Looks alright to me.

drunken monkey’s picture

Status: Needs review » Reviewed & tested by the community

Works fine for me, too.

cpliakas’s picture

Status: Reviewed & tested by the community » Fixed

Two testers, and good ones at that. Works for me! Committed at 89cc84b.

Thanks for testing,
Chris

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Added summary up to issue #13.