Coming from the following issues:

- 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 the (bogus) Input formats module attempt

we have to provide a machine name for D7 text formats. Without any further logic applied, just to be used as unique reference.

This patch requires #902644: Machine names are too hard to implement. Date types and menu names are not validated

Files: 
CommentFileSizeAuthor
#92 drupal.filter-alter.92.patch3.51 KBdagmar
PASSED: [[SimpleTest]]: [MySQL] 26,797 pass(es).
[ View ]
#88 drupal.filter-alter.88.patch3.41 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#86 drupal.filter-alter.86.patch3.18 KBdagmar
PASSED: [[SimpleTest]]: [MySQL] 26,764 pass(es).
[ View ]
#84 drupal.filter-alter.84.patch3.57 KBdixon_
PASSED: [[SimpleTest]]: [MySQL] 26,634 pass(es).
[ View ]
#82 drupal.filter-alter.82.patch3.21 KBdagmar
PASSED: [[SimpleTest]]: [MySQL] 26,643 pass(es).
[ View ]
#75 drupal.filter-alter.75.patch1.56 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 26,514 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#73 drupal.filter-alter.73.patch1.52 KBdagmar
PASSED: [[SimpleTest]]: [MySQL] 26,555 pass(es).
[ View ]
#71 drupal.filter-alter.71.patch1.55 KBdagmar
PASSED: [[SimpleTest]]: [MySQL] 26,516 pass(es).
[ View ]
#70 drupal.filter-alter.70.patch1.12 KBsun
PASSED: [[SimpleTest]]: [MySQL] 26,476 pass(es).
[ View ]
#65 drupal-filter-format-machine-name-65.patch19.25 KBdixon_
FAILED: [[SimpleTest]]: [MySQL] 26,441 pass(es), 6 fail(s), and 5 exception(es).
[ View ]
#60 drupal-filter-format-machine-name-60.patch18.69 KBdixon_
PASSED: [[SimpleTest]]: [MySQL] 25,720 pass(es).
[ View ]
#30 drupal.filter-format-machine-name.30.patch20.8 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 25,374 pass(es), 6 fail(s), and 5 exception(es).
[ View ]
#28 drupal.filter-drupal_alter-2.patch1.01 KBdagmar
PASSED: [[SimpleTest]]: [MySQL] 25,325 pass(es).
[ View ]
#26 drupal.filter-format-machine-name.26.patch18.69 KBdagmar
PASSED: [[SimpleTest]]: [MySQL] 25,365 pass(es).
[ View ]
#26 drupal.filter-drupal_alter.patch723 bytesdagmar
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in filter.module.
[ View ]
#13 drupal.filter-format-machine-name.13.patch17.45 KBdagmar
PASSED: [[SimpleTest]]: [MySQL] 24,877 pass(es).
[ View ]
#11 drupal.filter-format-machine-name.11.patch17.45 KBmlncn
FAILED: [[SimpleTest]]: [MySQL] 24,839 pass(es), 0 fail(s), and 12 exception(es).
[ View ]
#7 drupal.filter-format-machine-name.7.patch18.63 KBsun
PASSED: [[SimpleTest]]: [MySQL] 24,708 pass(es).
[ View ]
#6 drupal.filter-format-machine-name.6.patch15.83 KBsun
PASSED: [[SimpleTest]]: [MySQL] 24,708 pass(es).
[ View ]
#4 drupal.filter-format-machine-name.4.patch15.59 KBsun
FAILED: [[SimpleTest]]: [MySQL] 23,517 pass(es), 6 fail(s), and 1 exception(es).
[ View ]
#2 drupal.filter-format-machine-name.2.patch6.92 KBsun
FAILED: [[SimpleTest]]: [MySQL] 23,407 pass(es), 31 fail(s), and 58 exception(es).
[ View ]
drupal.filter-format-machine-name.0.patch6.88 KBsun
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

Status:Needs review» Needs work

The last submitted patch, drupal.filter-format-machine-name.0.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new6.92 KB
FAILED: [[SimpleTest]]: [MySQL] 23,407 pass(es), 31 fail(s), and 58 exception(es).
[ View ]

Stupid typo.

Status:Needs review» Needs work

The last submitted patch, drupal.filter-format-machine-name.2.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new15.59 KB
FAILED: [[SimpleTest]]: [MySQL] 23,517 pass(es), 6 fail(s), and 1 exception(es).
[ View ]

Status:Needs review» Needs work

The last submitted patch, drupal.filter-format-machine-name.4.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new15.83 KB
PASSED: [[SimpleTest]]: [MySQL] 24,708 pass(es).
[ View ]

Not sure how many stupid copy and paste mistakes I can make in a single patch :(

sun’s picture

StatusFileSize
new18.63 KB
PASSED: [[SimpleTest]]: [MySQL] 24,708 pass(es).
[ View ]

To be fully exportable, the correlating text format permission name must be based on the machine name, not on the serial format ID. That change is also no problem, because everything should use the API function filter_permission_name() to retrieve the permission name for a format. Code that does not is broken code.

dagmar’s picture

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?

sun’s picture

Why?

sun’s picture

Caching the results is tackled in #852470: Cache filter/format queries already. See you over there :)

mlncn’s picture

StatusFileSize
new17.45 KB
FAILED: [[SimpleTest]]: [MySQL] 24,839 pass(es), 0 fail(s), and 12 exception(es).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, drupal.filter-format-machine-name.11.patch, failed testing.

dagmar’s picture

Status:Needs work» Needs review
StatusFileSize
new17.45 KB
PASSED: [[SimpleTest]]: [MySQL] 24,877 pass(es).
[ View ]
<?php
        _update_7000_user_role_grant_permissions
($format_role, array('use text format ' . $format->machine_format), 'filter');
?>

should be

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

Status:Needs review» Needs work

The last submitted patch, drupal.filter-format-machine-name.13.patch, failed testing.

dagmar’s picture

Status:Needs work» Needs review
sun’s picture

Latest patch looks fine to me. Note that we're waiting for #902644: Machine names are too hard to implement. Date types and menu names are not validated to land, as this patch depends on it. (and we need to remove the @todo line in this patch afterwards)

mlncn’s picture

Status:Needs review» Postponed

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.

Damien Tournoud’s picture

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.

sun’s picture

Actually, most machine names are supposed to never change, as that would contradict their intention. As of now, the standard validation introduced in #902644: Machine names are too hard to implement. Date types and menu names are not validated invokes a callback to check whether a machine name already exists, and in case it does, it throws an form validation error. Perhaps that behavior could be optionally relaxed for UX purposes -- e.g., we could add a new optional property for the #machine_name definition, which would make the validation smarter, i.e., check whether the machine name exists, and if it does, append a _1 suffix, check again, try once more with _2, aso. until it's available and lastly override the submitted form value, without throwing a validation error.

q0rban’s picture

.

q0rban’s picture

Priority:Normal» Major

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.

dagmar’s picture

Status:Postponed» Needs review

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?

sun’s picture

Although we all disagree, that's probably a good idea, as this issue is required to unblock progress on #914458: Remove the format delete reassignment "feature" (it's RTBC for a small UI tweak only and will revert to needs work)

- Let's already use a #machine_name property on the element itself (i.e., as custom private property), following the proposed format. Add the #machine_name property value as JS setting.

- Copy the form_element_validate_machine_name() handler from the other patch as filter_admin_element_format_validate(), without any changes.

Therefore, if we manage to convince Dries/webchick of #type machine_name (and I seriously hope so), then we can simply change the #type and drop all other custom additions.

webchick’s picture

Version:7.x-dev» 8.x-dev

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.

sun’s picture

Version:8.x-dev» 7.x-dev

No, this patch is actually required to solve the last remaining problem in #914458: Remove the format delete reassignment "feature"

dagmar’s picture

StatusFileSize
new723 bytes
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in filter.module.
[ View ]
new18.69 KB
PASSED: [[SimpleTest]]: [MySQL] 25,365 pass(es).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, drupal.filter-drupal_alter.patch, failed testing.

dagmar’s picture

Status:Needs work» Needs review
StatusFileSize
new1.01 KB
PASSED: [[SimpleTest]]: [MySQL] 25,325 pass(es).
[ View ]

ops

sun’s picture

The drupal_alter() does not resolve anything. Proper machine names are required for #914458: Remove the format delete reassignment "feature" as well as handling of formats exported into code.

dagmar’s picture

StatusFileSize
new20.8 KB
FAILED: [[SimpleTest]]: [MySQL] 25,374 pass(es), 6 fail(s), and 5 exception(es).
[ View ]

@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

Status:Needs review» Needs work

The last submitted patch, drupal.filter-format-machine-name.30.patch, failed testing.

sun’s picture

Title:Text formats need a machine name» Missing drupal_alter() for text formats and filters
Category:task» bug

@dagmar: Thanks for keeping this going! Unfortunately, it seems like this patch won't be accepted for D7. It would be great if you could try to get at least those two drupal_alter()s in, so we at very least can try to do something from contrib. The drupal_alter()s are the only remotely possible way, even though we'll have to come up with lots of extra validation and sanitation logic then. The future (and hopefully success) of this issue is in your hands now. I'll keep my fingers crossed.

sun’s picture

Assigned:sun» Unassigned

Sorry, forgot to unassign.

febbraro’s picture

subscribe (and +1)

arianek’s picture

subscribe

bonobo’s picture

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.

chrisshattuck’s picture

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?

sun’s picture

To clarify the (only) possible way forward for D7:

- #902644: Machine names are too hard to implement. Date types and menu names are not validated is no longer on the table for D7. It will be (moved and) provided by http://drupal.org/project/core instead.

- The original patch of this issue is also no longer on the table for D7. It will also be provided by http://drupal.org/project/core.

- To actually be able to export anything, the two invocations of drupal_alter() in above patch is the only possible way. Without those invocations, nothing is possible at all.

So if there is anything that can be done here, then it's only the drupal_alter() invocations. I'm not sure how Drupal core developers will resolve the last bits of #914458: Remove the format delete reassignment "feature", but that's no longer my beer; can only guess it will likely be a very dirty hack, which I'd never ever approve.

jhedstrom’s picture

subscribing

dagmar’s picture

Title:Missing drupal_alter() for text formats and filters» Text formats need a machine name
Version:7.x-dev» 8.x-dev
Category:bug» task

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.

chrisshattuck’s picture

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?

eaton’s picture

Title:Text formats need a machine name» Missing drupal_alter() for text formats and filters
Version:8.x-dev» 7.x-dev

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.

dixon_’s picture

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.

bonobo’s picture

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.

David_Rothstein’s picture

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?

bonobo’s picture

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.

q0rban’s picture

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". :)

eaton’s picture

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.

dixon_’s picture

@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. :)

eaton’s picture

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."

q0rban’s picture

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. :)

dagmar’s picture

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

<?php
 
'format' => 1
?>

should change to

<?php
 
'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).

irakli’s picture

@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.

David_Rothstein’s picture

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?

alex_b’s picture

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.

webchick’s picture

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’s picture

Text formats are not the last missing piece for being fully able to export all core configuration. There are also user roles and permissions, which definitely won't be tackled for D7.

That is, because there's not even a patch for user roles/permissions that could be remotely considered as "ready".

I fully understand that it is basically too late for the originally proposed improvement -- although this particular patch (of this issue) does not and should not negatively affect any modules or code in the wild, because it is only changing code and logic that is internal to the Filter API. If some code happens to be broken by this change, then that code is broken in the first place, as it doesn't use the Filter API correctly (and we do not babysit broken code).

Truth must be told, however. This patch is only a small step towards the goal. It merely allows to "map" serial IDs to machine names, and nothing else. All code in core and contrib will still use text format IDs to reference a particular format, and even the tiniest thought about changing that is totally, utterly, and completely out of scope and sync for D7. This patch only allows text formats to be exportable in the first place, leaving the heavy duty of figuring out whether format ID 2 is actually the expected machine name "two" to the implementation that tries to export and import stuff from code in the first place.

However, without this very basic facility in core, it is close to impossible to achieve that same id/machine_name mapping reliably otherwise. It is a very small step in the right direction only.

webchick’s picture

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.

irakli’s picture

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 :)

dixon_’s picture

Status:Needs work» Needs review
StatusFileSize
new18.69 KB
PASSED: [[SimpleTest]]: [MySQL] 25,720 pass(es).
[ View ]

Here is a re-roll of #26.

alex_b’s picture

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.

dagmar’s picture

Status:Needs review» Needs work
+++ 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:

<?php
   
'#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.

sun’s picture

Status:Needs work» Postponed

I'm postponing this issue for now on the results of the discussion in #933846: Better distro support. Based on decisions taken, further work on this might make sense or not.

chx’s picture

Status:Postponed» Needs review

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

dixon_’s picture

StatusFileSize
new19.25 KB
FAILED: [[SimpleTest]]: [MySQL] 26,441 pass(es), 6 fail(s), and 5 exception(es).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, drupal-filter-format-machine-name-65.patch, failed testing.

dixon_’s picture

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

sun’s picture

This issue should focus on the drupal_alter() only. Text format machine names are already tackled in #934050: Change format into string.

dixon_’s picture

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

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new1.12 KB
PASSED: [[SimpleTest]]: [MySQL] 26,476 pass(es).
[ View ]

Attached patch is all that remains. However, we need to wait for #934050: Change format into string, as the added filter_format_exists() over there needs the drupal_alter(), too.

dagmar’s picture

StatusFileSize
new1.55 KB
PASSED: [[SimpleTest]]: [MySQL] 26,516 pass(es).
[ View ]

Included drupal_alter for filter_format_exists

sun’s picture

Status:Needs review» Needs work

I don't think it's a good idea to use a separate alter hook. In http://drupal.org/node/934050#comment-3609962, I already wanted to prepare filter_format_exists() for this issue, but had to remove that code.

dagmar’s picture

Status:Needs work» Needs review
StatusFileSize
new1.52 KB
PASSED: [[SimpleTest]]: [MySQL] 26,555 pass(es).
[ View ]

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

sun’s picture

+++ modules/filter/filter.module
@@ -306,7 +306,15 @@ function filter_format_disable($format) {
+  $formats = db_select('filter_format', 'ff')

@@ -415,6 +423,10 @@ function filter_formats($account = NULL) {
+      drupal_alter('filter_formats', $formats['all']);

Hm, no. The entire point of retrieving an array of formats is that we can invoke the very same drupal_alter() hook in filter_format_exists().

Powered by Dreditor.

dagmar’s picture

StatusFileSize
new1.56 KB
FAILED: [[SimpleTest]]: [MySQL] 26,514 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Oh, I see.

sun’s picture

Status:Needs review» Reviewed & tested by the community

Thanks! Looks ready to fly for me now.

sun’s picture

Status:Reviewed & tested by the community» Needs review

oopsie, no, we're missing API docs for the two alter hooks.

Status:Needs review» Needs work
Issue tags:-export

The last submitted patch, drupal.filter-alter.75.patch, failed testing.

dagmar’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work
Issue tags:+export

The last submitted patch, drupal.filter-alter.75.patch, failed testing.

webchick’s picture

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?

dagmar’s picture

StatusFileSize
new3.21 KB
PASSED: [[SimpleTest]]: [MySQL] 26,643 pass(es).
[ View ]

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.

dagmar’s picture

Status:Needs work» Needs review

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

dixon_’s picture

StatusFileSize
new3.57 KB
PASSED: [[SimpleTest]]: [MySQL] 26,634 pass(es).
[ View ]

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.

David_Rothstein’s picture

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....

dagmar’s picture

StatusFileSize
new3.18 KB
PASSED: [[SimpleTest]]: [MySQL] 26,764 pass(es).
[ View ]

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)

David_Rothstein’s picture

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.

dagmar’s picture

StatusFileSize
new3.41 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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

Status:Needs review» Needs work
Issue tags:-export

The last submitted patch, drupal.filter-alter.88.patch, failed testing.

dagmar’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work
Issue tags:+export

The last submitted patch, drupal.filter-alter.88.patch, failed testing.

dagmar’s picture

StatusFileSize
new3.51 KB
PASSED: [[SimpleTest]]: [MySQL] 26,797 pass(es).
[ View ]

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

dagmar’s picture

Status:Needs work» Needs review
alex_b’s picture

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.

webchick’s picture

Version:7.x-dev» 8.x-dev

Then this sounds like D8.

YesCT’s picture

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.)

xjm’s picture

Tagging issues not yet using summary template.

langworthy’s picture

subscribe

langworthy’s picture

#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?

langworthy’s picture

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.

catch’s picture

Category:task» feature
Priority:Major» Normal
langworthy’s picture

Issue tags:+needs backport to D7

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?

sun’s picture

Status:Needs review» Closed (won't fix)

This won't happen, as we're moving text formats into configuration and configuration cannot be altered.

markcarver’s picture

Version:8.x-dev» 7.x-dev
Status:Closed (won't fix)» Reviewed & tested by the community
Issue tags:-needs backport to D7

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.

David_Rothstein’s picture

Issue summary:View changes
Status:Reviewed & tested by the community» Needs review

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).