Missing drupal_alter() for text formats and filters

Question. Have you considered to use a cache in filter_formats, like other modules does (views, imagecache, strongarm, etc)?

@@ -384,6 +385,10 @@ function filter_formats($account = NULL)
       ->orderBy('weight')
       ->execute()
       ->fetchAllAssoc('format');
+
+    // Allow modules to alter the list of formats; e.g., to load formats
+    // exported into code.
+    drupal_alter('filter_formats', $formats['all']);

As far I know, every time a different user try to post a comment, this drupal_alter will be called, isn't?

Rerolled to account for user module renaming an internal update function, _update_user_role_grant_permissions to _update_7000_user_role_grant_permissions.

Other than that, it applies, functions, and is RTBC.

        _update_7000_user_role_grant_permissions($format_role, array('use text format ' . $format->machine_format), 'filter');

should be

        _update_7000_user_role_grant_permissions($format_role, array('use text format ' . $format->machine_name), 'filter');

Excellent. And thanks dagmar, i'm not sure what combination of undo, control z, control where-did-i-put-my-brain did that before i rolled the patch.

In relation to #914458: Remove the format delete reassignment "feature", note that this patch will have to change the machine name of a deleted text format to something unique (for example 'deleted ' . $format->id) when a text format is disabled.

.

I'm going to bump this up in priority. Used improperly, input formats are one of the biggest security vulnerabilities in Drupal, and the fact that exportables continue to gain momentum and input formats are currently keyed via a serial ID everywhere in Drupal is almost terrifying. Serial keyed input formats will increasingly present security vulnerabilities, as entities (CCK fields, Views, etc.) that specify a input format exported from Site A and then are imported into Site B may lead to users being allowed to input malicious content.

As far I know, @webchick is not thinking in commit #902644: Machine names are too hard to implement. Date types and menu names are not validated, In case #902644 will postponed to D8, should we provide this patch with a simple machine handler included in it?

I'm really sorry to keep repeatedly banging the same drum over and over (believe me, I know it gets as tiring for you as it does for me), but there is absolutely nothing justifying making this change in D7.

Formats are hard to export? Well, yes. They've been hard in D6, and apparently will continue to be hard in D7 since we're a year past when it was appropriate to make these kinds of changes.

People can screw up input formats? Well, yes. This has been a problem since time immemorial. Hopefully a lot of the UI changes to text formats that sun, David Rothstein, and others made this release will help with that.

Rerolled since #852470: Cache filter/format queries was committed. This patch doesn't includes items described in #23.

And also, as a final intent to allow export input_formats and provide machine names to modules I think only may be necessary the drupal_alter in filter_formats(). (Then features can provide machine names using hook_schema_alter) See drupal.filter-drupal_alter.patch

ops

@sun, yes I know, is a last option patch.

Ok, here are the changes suggested in #23, since there is no function called 'filter_format_load_by_machine' I'm not using the 'exists' property in #machine_name

subscribe (and +1)

subscribe

Subscribe and +1

Despite the fact that, as webchick points out in #24, "we're a year past when it was appropriate to make these kinds of changes" in D7, not getting this in means that a component of install profiles will be more difficult than it needs to be, which in turn relegates install profiles to something that will continue to be focused on specific use cases for the next 2+ years (the lifecycle of D7, estimated conservatively - it could easily be longer).

Solving this now means that more people will be able to build install profiles more easily using D7. This is a Very Good Thing, and would help adoption of Drupal at all levels. Working around the lack of machine names added - easily - a week's worth of development to building our install profile.

Given how this change would simplify building and reusing exportables, and how, in turn, this would make it easier to package more complex config into reusable building blocks, it's not a stretch to categorize this as a usability issue, and that not addressing this in D7 is a critical bug.

From the comments in #24, #25, and #32 it's unclear what the status of this is for D7, and what the barriers are (aside from time) from getting this in. If there is a chance that this can get in to D7, we will gladly put time and hours into testing this for D7. The lack of this in D6 was an incredible hassle, and I would like to not relive that pain for the entire D7 lifecycle.

Quick summary before I throw in my 2 cents

* This patch leverages a patch that Adds a machine name field to the forms API.
* This patch, in turn, offers one solution to this patch which removes the 'feature' that moves content to a default filter when the filter it's using is deleted by disabling the filter instead of deleting it, and using the machine_name field to check for conflicting machine names.
* However, the push for getting a machine_name field type is getting nixed for D7 - not because anyone has a beef with it per se, but rather because it's too late in the game for API changes in D7, so it's being pushed to D8
* But, maybe some drupal_alters can get thrown in so contrib doesn't have such a hard time layering exportables onto core.
* To move forward with the drupal_alter() option, we'd first need a working version of this patch and then someone to RTBC.

The question, as nedjo points out here, is if this patch can be viewed as a security fix that also solves a couple other critical issues helping to move things forward, or if this is an API change that can't be justified 12 months into a code freeze. Everyone seems to agree that the patch is a good idea, just maybe not for D7.

My 2 cents

From my limited experience, it seems like exportables are going to a big deal for the life of D7, and that any DX improvements in this area that are ready to roll will really pay off in added sanity and time for developers. It will free devs up to focus on other important issues for D8. But, I believe that strong leadership is critical for any project, and if anyone has earned leadership by gaining the broadest perspective of the implications of any one patch for the Drupal project as a whole, it's the core committers. So, I personally feel that deferring to this leadership - albeit after reasonable debate - is healthy and important.

There are also workarounds for this one. The drupal_alters() is something, and the Input Formats module provides another kind of model (though sun suggests this module is bogus, how come?).

I would be interested to hear what workaround people would choose if this patch doesn't go through. Would you use modules like Input Formats that create a layer on top of the un-exportable component? Will folks agree that this is a critical enough patch that they will build it into their sites and distributions anyway? What's the next best thing? Would that make an impact on the decision to move this patch through?

subscribing

Well, if we are going to do this, let's do this in the better way possible.

We all know that text formats needs machine names, as sun, chrisshattuck, q0rban, and other said, there are a lot of advantages to have machine names here.

However, I wouldn't like to hide this patch a as bug, this is not a bug is a feature request, yes we know that this missing feature is causing a lot of issues, but don't make harder the task for committers and leave this issue as the original request.

Like @webchick explained in a recent post, this is not time to alter the schema and convert a lot of parts of drupal to provide this kind of feature.

For other side, if case you don't know, I'm the current maintainer of Input Formats module. As sun said in #0, the module is a bit bogus, it is working pretty fine but limitations of D6 make this not so good as expected. (See http://drupal.org/node/643566#comment-3412656 fore more information about this problems) But we can fix this problem before D7 gets released.

To summarize, it is too late to put this in D7, however machine names could be provided by a contrib module. We just need to drupal allow to alter (via drupal_alter) the text formats definition.

And here we have another issue, like other modules does, load definitions from code usually requires a cache (see Views, Strongarm,Imageache, Context as examples). There is another issue to provide a cache for text formats here #852470: Cache filter/format queries but Moshe Weitzman thinks is not correct and 'hurts' drupal.

I'm not capable to discuss with Moshe about this issue. He probably have his reasons to said what he said, I'm just asking to people with more experience in drupal development to determine if patch that I'm posting on this issue can or cannot go into D7. Maybe not in beta, but in rc or 7.0. Allowing to contrib to define machine names for text formats and avoiding to wait until Drupal 8 to get a elegant solution for this complicated issue.

Thanks for reading.

Are there any other recent D7 issues you've come across that involve adding drupal_alter() somewhere? Does that constitute an API change, since it adds a new hook? If not, then if those two alters are in the right place to allow contrib to take over until D8, it would be awesome to sink those in. Also, should that patch be teased out into a separate issue?

dagmar, if I'm out of line feel free to throw something at me, but your post suggests that there still is a critical D7 related fix that is needed to at least offer a stop-gap for this problem. That is, the drupal_alter() workaround.

One of the reasons I'm concerned about this is the complexity of building profiles and distros for end users who aren't experienced site builders -- if any input format alterations need to be made for security reasons, the distro maintainers are in a very difficult spot. While it's possible to make one in code, referring to it in Views, CCK formatter settings, and so on is essentially impossible. As the maintainer of Input Formats that's probably not a shock to you, but I just wanted to point out that there's a very real concern about the difficulty of shipping 'secure from the moment you install it' distros built on top of Drupal without something.

That's also why (IMO at least) this is a bit more critical than normal configuration exportability -- stuff like Menu items etc are obviously going to be a beast, and we don't want to start another ripple effect of patches, but input formats are nigh impossible to avoid if you're setting up a site that needs to be secure for multiple users.

I agree on the security issues. Distros are important for the future of Drupal. And leaving security holes out-of-the-box is not good. Drupal has a reputation of high security. Let's stick to that. And thus, we at least need the possibility of fixing this in contrib. As @eaton says:

I just wanted to point out that there's a very real concern about the difficulty of shipping 'secure from the moment you install it' distros built on top of Drupal without something.

The drupal_alter() is not a big change for core IMO, but a huge win for distros, exportability and contrib when other approaches aren't possible at this time! Without this it's almost impossible to do anything as @sun points out several times.

So huge +1 for this.

In #42, eaton says:

One of the reasons I'm concerned about this is the complexity of building profiles and distros for end users who aren't experienced site builders

Building profiles and distros are complex for experience site builders. Features take some of the sting out of building profiles, but right now, to make the most out of features, they are essentially tied to a single base install profile that sets some of the values that require a serialized ID, and doesn't have a machine name. We can push these values to code, but as features and exportables continue to grow in popularity, they will be used by people who don't understand how/why these pieces fit together. And someone will install a feature that doesn't work, and it will lead to another flavor of "Drupal sucks" conversations for the D7 lifecycle.

In #21, q0rban says:

Used improperly, input formats are one of the biggest security vulnerabilities in Drupal, and the fact that exportables continue to gain momentum and input formats are currently keyed via a serial ID everywhere in Drupal is almost terrifying.

In site A, input format 4 could be filtered html. In site B, input format 4 could allow PHP input. Machine names solve that.

D7 will see more and more install profiles, and we want that. That's how Drupal will take over the web.

This patch allows experience site builders who *aren't* developers to share their work. It would be great to not push this off for another 2-3 years until D8.

Obviously the general idea of a machine name is great. I am going to stay out of the D7 vs D8 debate personally :)

However, I just wanted to jump in and say that I don't understand the security arguments and was wondering if someone could try to explain them further. Or more specifically, I understand the security issue that you face if you try to migrate text format data between sites, but I don't understand how a machine name solves that.

For example, this:

In site A, input format 4 could be filtered html. In site B, input format 4 could allow PHP input. Machine names solve that.

I do not understand that because even if site A stores a text format called "filtered_html" and site B also stores a text format called "filtered_html", that does not mean they are the same text format; they could very well be configured with a different set of filters and a different set of options.

The only way to know if two text formats are actually identical between two sites is to look at the exact list of filters they contain and make sure everything is the same... no?

Using a machine name, you could export all of the config with the input format, so you could know (or at least be more certain) that your input formats were identical between sites - when you export the view "show_this_data" you're not exporting the name, you're exporting the config options of the view that come with the name.

This was one of the things we wrestled with while building our profile, and why we stuffed most of this config into code within the profile, as opposed to managing it with a feature. However, this ties our features to this base profile, which means that they are less reusable than they should be.

We can work around serialized IDs - we have in the past, and if we have to, we'll continue to do so in the future. But getting this in now would mean that site builders who aren't developers could contribute more meaningfully in this space for the 2-3 years that D7 is out.

The only way to know if two text formats are actually identical between two sites is to look at the exact list of filters they contain and make sure everything is the same... no?

David, yup, you are totally right. Adding machine names to input formats certainly doesn't disallow someone from unwittingly opening a security vulnerability, however IMO it is a huge step in the right direction. For instance, if you have a CCK field that uses input format "filtered_html", it won't mistakenly get lined up with "my_stupid_input_filter_that_allows_anything_including_php_code_and_magic_spells". :)

The only way to know if two text formats are actually identical between two sites is to look at the exact list of filters they contain and make sure everything is the same... no?

You're correct, and the only real solution to this problem is to have your code create a fresh input format from scratch, for its purposes, and use it when needed. The problem is that while your module can stash the id of that filter format and use it later, nothing else in Drupal is smart enough to use it. Have a View whose 'empty text' includes potentially unsafe markup? There's no way to reference it. Have a view that needs to pipe a field through that input format? No way to reference it, since Views doesn't know about the secret place where you've stored that filter's fid. And so on and so forth.

drupal_alter() won't solve that inherently, but it will allow a third-party module like Input Formats to graft on a standardized machine name in contrib that other 'well behaved' modules can take advantage of. It's not as good as having an actual machine name, but it at least leaves the potential for a solution.

@David_Rothstein

The security issues may appear if you ship exported content types configured for a specific input format. If that format does not exist on the site, it will fall back to the default format (correct me if I'm wrong). Or the exported content types will line up with a format of the same incremental id already existing on the site. This is an unpredictable behavior that potentially may be dangerous. The default format is most often configured safely (hopefully). But let's say that the exported content type's format lines up with an already existing format that happens to be Full HTML. Not good. :)

There are a couple different flavors of this problem, and that's definitely one of them. Basically, "without some solution, input formats other than filtered html and full html cannot be used reliably in distros."

Forgive me if I'm missing something, but I don't see how a drupal_alter is going to solve this problem. Sure it makes it easier for exporting input formats, etc., but it doesn't solve the problem of exporting Views, Fields, Boxes, etc., unless every one of those modules is going to declare a dependency on whatever module in contrib will be making the fix. Until filter_format_load() actually accepts a string machine name, we are effed. Until D8. :)

Implications of make text formats trully exportables are pretty bigs, as q0rban said, other modules should change the way in they reference to text formats, views is a clear example of that

  'format' => 1

should change to

  'format' => 'filtered_html'

IMO this is too much to put it into D7. I think the paragraph to resume this issue in D7 is what eaton said in #48:

drupal_alter() won't solve that inherently, but it will allow a third-party module like Input Formats to graft on a standardized machine name in contrib that other 'well behaved' modules can take advantage of. It's not as good as having an actual machine name, but it at least leaves the potential for a solution.

@webchick said in #24 that she doesn't see a good reason to put this change in drupal 7, dixon_ in #43 support the inclusion of a few drupal_alter as a partial solution.

Personally I prefer something workable from contrib than nothing (or bogus like input formats module does at this momment).

@webchick,

if I may chime-in, and contribute my two cents from the perspective of somebody who has maintained large distros in D6 and is working on their migration to D7...

I definitely understand and respect your stand that it's too late in the game for what-seems-like an architectural change when the community is struggling with the last remaining critical issues. And I can see how you don't see this particular issue as being a critical issue.

However...

In any large effort there're operational considerations (e.g.: "let's not drag-out release by introducing new changes") and then there are strategic considerations. Strategic here is the goal to make D7 the version in which distros will _really_ take off. We learned many things that really did not work in D6 for distros. Lack of "globally unique" machine names is #1 problem among those because it stands in the way of making things properly exportable. Drupal7 is our chance to fix this major problem.

Considering how important timing is in the market (2011 will be the most important year, to date, for Drupal and it will be up to D7), and the reality of every future major Drupal version probably taking longer to release, I am afraid D8 will simply be too late.

In conclusion, as much as I understand practical, tactical reasons why you don't see it reasonable to make changes like this before the release, I believe strategic considerations do override those in a (small and specific) number of cases. And this is one such, important case.

I join my vote for including this change as part of D7.

Thank you.

The security issues may appear if you ship exported content types configured for a specific input format. If that format does not exist on the site, it will fall back to the default format (correct me if I'm wrong). Or the exported content types will line up with a format of the same incremental id already existing on the site. This is an unpredictable behavior that potentially may be dangerous.

In D7 it actually falls back to displaying an empty string. (But yes, if it lines up with another format, that of course can be dangerous.)

So it sounds like the overall conclusion is that the potential security issues with exporting text+formats between sites can't be solved fully in D7. But the changes proposed here could allow contrib modules to experiment better with the idea.

The alter hooks proposed here sound relatively harmless to me. (Although nothing is totally without consequence; allowing this probably would affect any module which wants to join against the {filter_format} table?)

Just to cover all bases, is there any way to do this without new alter hooks? In the other issue, I think, @webchick suggested instead of new hooks, adding a tag to the existing database queries so contrib modules could modify them via hook_query_alter(). Is that not enough to help, or is a full alter hook needed?

A second irakli. There are tactical considerations.

The larger exportables issue was brought up in Paris where it was deemed too large of a change at that late (!) in the release cycle. This was over a year ago. In the meantime exportables have firmly established themselves as one of the main techniques for building distros. Moreover, the ability to create distros with Drupal has been identified as a way to diversify and grow Drupal. We've also seen great steps in the right direction in core - namely vocabulary machine names.

If we can't fix exportability issues like this one in 7, we will have to figure out how to fix this before Drupal 8. We can't wait that long. Maintaining workarounds in contrib is expensive.

We're using this patch on a site build right now, we'll report back with a review. Thanks for everybody's hard work.

Well, I was of course there for that discussion. ;) The lateness of it was definitely one thing. I was approached about it in the BoF room on like September 5 or something, and feature freeze was Sept. 1. Another was that we wanted to take the very first working stab we had at an exportability API (which hit 1.0 like the week or two before Drupalcon Paris, iirc) and lock it into the core product for 3+ years. I viewed this as risky; akin to taking Views 1 and locking it unto core for 3+ years. Earl agreed with me, as well, from what I remember, though I could be mistaken. I do remember that I somehow left that room still breathing, so there's that. ;)

Now that it's been a year, export.inc in CTools has largely gone unchanged, and is used more widely throughout contrib by those "in the know," which is great. But it was impossible to know that back then, and I still think that was the right move given the information that was available at the time. And I do still think that we might get an export.inc 2.0 in contrib at some point that addresses some of the limitations in the current system once more people find out about this paradigm shift and work on improving the tools.

Now, in September (now October) 2010, the problem with it now is that it is really, really too late. Not only are we a year past API freeze, but we're actively trying to get a beta out the door within the next few days, and a release within the next couple of months. So jaunting off on a nice "Let's make everything in core exportable" initiative now is not going to fly.

So I don't disagree on the arguments for strategic importance. But I argue that it is even more strategically important to get Drupal 7 effing done. For a variety of reasons, including core team burn-out, evolution in our "competitor" space, and so on. I also find it very frustrating that this issue has the distinct odor of an e-mail being sent around to 50 people, many of whom were totally uninvolved during the entire Drupal 7 release process, saying "Go +1 this." Bleh.

But at any rate. So, we already made vocabularies exportable. If we make input formats exportable, is that where it ends? Or are we launching on a 3+ month project to go and add machine names to every little thing?

sun: Ok, that makes sense.

Where I'm coming from is that until I know the full scope of what we're talking about here, it's incredibly risky for me to agree to anything outside of the narrow funnel that we discuss over at #927792: Please explain what is still considered for D7. Our core development team is suffering from major burn-out, and our competitors are evolving into our space. Drupal 7 needs to get out there, and soon, for the future of the project. And, as we all know from various client work, one of the only ways to get a lagging project out the door sooner is to cut scope. Ruthlessly.

So alex_b and I discussed this on IRC and came up with the following plan.

alex_b: You and the rest of your cohorts put your heads together and come up with The Definitive Guide To Making Drupal Core Not Stab People Directly In The Face When Trying To Build Distributions
 alex_b: Then Dries and I can evaluate that list and figure out if we fix those by:
1. Directly changing them in core
2. Offering workarounds for contrib
3. Saying "Sorry, that's just a limitation we're going to have to live with for another release"
4. Street Fighter II

Alex is going to try and get this list together by the end of the week. Beta will hopefully be out before then.

That's probably off-topic for this particular issue, but since this is where everyone decided to pig-pile, posting it here.

And the prize for keeping a great sense of humor even in the most heated Drupal discussion (which statistically would be a 10x size of a regular heated discussion) goes to: @webchick. :)

P.S. I do accept "bleh" :) but in my defense there's only time for so much in life: you either build modules and distros or are actively involved in core. Have not figured-out a good way for doing both. Will try to work on it for D8 :)

Here is a re-roll of #26.

I've started the plan webchick mentioned in #58: #933846: Better distro support

This is not "The Definitive Guide..." yet (will it ever be possible to write one?), but it's what I think we should get done for Drupal 7. Your input is much appreciated.

+++ modules/filter/filter.admin.inc	5 Oct 2010 21:31:07 -0000
@@ -122,6 +126,14 @@ function filter_admin_format_form($form,
+    '#machine_name_exists' => 'filter_format_load',

This is not correctly implemented. test pass since this function is never called. (Due it requires #902644: Machine names are too hard to implement. Date types and menu names are not validated)

Here is how this should be done:

    '#machine_name' => array(
      'exists' => 'filter_load_by_machine_name',
    ),

Also, we need a new function that check if a text format exists based in machine_name, filter_format_load uses an numeric id.

Powered by Dreditor.

Instead, get Dries in the issue and get approval on the schema change.

Here is a cleanup using the additions from #902644: Machine names are too hard to implement. Date types and menu names are not validated.

I followed dagmar's suggestion in #62 and added a new function called filter_format_load_by_machine_name(). It could probably need a static cache, although the overhead of that foreach loop shouldn't be that big. I also think that filter_format_load() and filter_format_load_by_machine_name() could me merged in some smart way. But I'll leave that for someone other to decide.

Here is a first try. I've tried it locally and it seems to work fine. But let's see what the bot thinks about it.

Hmm... guess we'll have too look into it a little bit more :)

Oh, sorry. I apparently missed some of the discussions in #933846: Better distro support. I'll refocus my work! Thanks :)

Included drupal_alter for filter_format_exists

Using code provided in http://drupal.org/node/934050

Oh, I see.

#75: drupal.filter-alter.75.patch queued for re-testing.

In addition to those missing docs, can someone remind me again why we need this now that the format IDs as strings and machine name patches went in?

Well, after read again the 40+ messages justifying the inclusion or not of drupal_alter here is a summary.

Originally, the inclusion of drupal_alter was a 'last minute' chance to allow other modules to alter the numeric id into a machine name. Also, this would enabled the inclusion of text formats from code.

Now that #934050: Change format into string is in, most part of justifications for this issue are not useful. However, as @eaton said in #42, and @dixon_ said in #43, the interesting part of this two new hooks is the possibility to load text formats from code.

At this moment, with #934050: Change format into string committed, features can export the whole definition of the text format, but it cannot load the definition from code once exported, due the two missing drupal_alter. This is what I suggested in #8.

As a final note, this patch allows to:

1) Load (already exportable) text formats from code.
2) Export in a single feature, a text format and a wysiwyg profile, a big improvement in usability and re-use of features.
3) This issue fix also a lot of issues marked as 'won't fix' in D6,

Features: #643566: Text formats
Exportables: #640302: Provide exportable for input formats
Wysiwyg: #624018: Exportables and Features support for WYSIWYG 7.x
Better formats: #616496: Features integration

And put a new module into core, Input formats, that is not needed anymore with this patch.

So, I don't want to lie about the real need of this patch. But it would be really nice to see it in. A lot of people will be happy as @dixon_ said in #43, it is a really small modification.

And IMO, taken this from #933846: Better distro support this patch can be categorized in this way:

1) A fix is non-invasive; i. e. it does not change or only minimally changes existing APIs.
3) The issue is non-fixable outside of core.

Patch attached with the docs, it needs still some work.

Changing to 'needs review' to get some advices about the docs.

Small doc style adjustments.

+++ modules/filter/filter.api.php
@@ -185,6 +185,52 @@ function hook_filter_info() {
+ * @see filter_formats

Missing ()

+++ modules/filter/filter.api.php
@@ -185,6 +185,52 @@ function hook_filter_info() {
+ * @see filter_list_format

Also missing ()

Powered by Dreditor.

 function filter_format_exists($format_id) {
-  return (bool) db_query_range('SELECT 1 FROM {filter_format} WHERE format = :format', 0, 1, array(':format' => $format_id))->fetchField();
+  $formats = db_select('filter_format', 'ff')
+    ->fields('ff')
+    ->condition('format', $format_id)
+    ->execute()
+    ->fetchAllAssoc('format');
+
+  // Allow modules to alter the list of formats; e.g., to load formats
+  // exported into code.
+  drupal_alter('filter_formats', $formats);

This database query only pulls one row from the table, but hook_filter_formats_alter() implementations are going to expect to receive all formats as input, so this seems like an inconsistency?

Also, invoking the same alter hook from inside filter_formats() as inside this function basically means there would always be an inconsistency, with regard to whether or not any disabled formats are passed in to the hook....

I have re-rolled the patch including the change suggested by David_Rothstein. Now both calls to drupal_alter pass the array of text formats.

Related to the inconsistency, filter_format_exists() is an internal function used by the new form element of #type: 'machine_name' that uses this function to determine if a text format already exists.

In theory if a module implements hook_filter_formats_alter() and adds a new text format, call the same hook from filter_format_exists should be safe, after all this function only determine if the text format is defined (in the db or in code)

I think it's unexpected to have the same alter hook get inconsistent inputs passed in. Someone might be using this to actually alter things (not just to add a new one), and then it could lead to weird results.

Best (simple solution) I can think of is to make the hook take a second parameter, which is just a boolean that tells the person implementing the hook whether the passed-in list includes disabled formats? That's not perfect, but at least it gives them something they can check in their code to know what they are getting and therefore what they are supposed to be returning.

Thanks, @David_Rothstein here is the hook with new parameter.

#88: drupal.filter-alter.88.patch queued for re-testing.

Fatal error: Only variables can be passed by reference. Doh

From the perspective of making it easier for Features module to export filter configuration this patch is not needed (related issue #643566: Text formats).

hook_features_rebuild() reliably loads a text format into the database (if I am missing something please point me to the right issue).

Given that the drupal_alter() approach suggested here won't win us anything I would avoid it as it introduces a new pattern of managing configuration in code (CTools style loader functions and hook_features_rebuild() being the other patterns).

What was clutch for safe exportable text formats was #934050: Change format into string - this issue helped making that happen.

Then this sounds like D8.

I'm confused, in #643566: Text formats sun is waiting on this issue, and in this issue comment 94 makes it should like this issue is not needed to solve the problem and people should use 643566 ... which is the issue that is waiting on this issue. I'm reading circular logic and I need someone to to recap so I can sort out these two issues. (I'm wanting to make a wysiwyg feature. And it is frustrating that it is so hard to make it an exportable. I saw the lullabot how tohttp://www.lullabot.com/articles/wysiwyg-feature a while ago and was hoping to find some patches that would let me do that.)

Tagging issues not yet using summary template.

subscribe

#643566: Text formats seems to have fixed the problem that this issue originally set out to do (Give text formats a machine name for exportability).

I think this can be closed. Can someone confirm?

Following up on #99 I'm able to export text formats into a feature module using Drupal 7.7 and Features 7.x-1.0-beta3.

Ok. I re-read most of this thread and I *think* I get it now.

Comment #94 explains why this issue is not needed for Features module to export filter config.

Comment #82 explains why this issue is valid. It provides two hooks: hook_filter_formats_alter() and hook_filter_list_format_alter()

Should:

function hook_filter_formats_alter(&$formats, $only_enabled_formats) {
  $filters['custom_format'] = array(

be:

function hook_filter_formats_alter(&$formats, $only_enabled_formats) {
  $formats['custom_format'] = array(

Also I wonder how hook_filter_formats_alter() is different from hook_filter_default_formats_alter() that features.module provides?

Hmm, "won't fix" on 8.x doesn't mean it's still not necessary for 7.x.

Just applied patch in #92 to 7.x just fine.

Has anything changed here since alex_b's comment in #94?

Especially if this feature can never be added to Drupal 8, there would have to be a really good use case to add it to Drupal 7 (and then take it away from people in Drupal 8 once they have gotten used to it being there).