Enable modules to define theme engines

Done, and good to go.

Little things.

Also I wanted to ensure:

We do want to expose the functionality to modules _only_ for testing purposes or would it make sense to have a twig module, which then ships the twig engine without having to copy it to sites/all/themes/engines/twig?

If we only want to expose it for testing purposes the documentation needs some work to say the same as the hook_system_theme_info() description:

https://api.drupal.org/api/drupal/modules%21system%21system.api.php/func...

1. +++ b/modules/system/system.module
   @@ -2505,6 +2505,9 @@ function _system_update_bootstrap_status() {
   +  // Find theme engines
   +  $engines = drupal_system_listing('/^' . DRUPAL_PHP_FUNCTION_PATTERN . '\.engine$/', 'themes/engines');
   +
   
   @@ -2519,8 +2522,17 @@ function _system_rebuild_theme_data() {
   -  // Find theme engines
   -  $engines = drupal_system_listing('/^' . DRUPAL_PHP_FUNCTION_PATTERN . '\.engine$/', 'themes/engines');
   

   This code move looks unrelated.

2. +++ b/modules/system/system.module
   @@ -2519,8 +2522,17 @@ function _system_rebuild_theme_data() {
   +  // As Drupal is not able to find theme engines in modules, then, we allow
   +  // users to add theme engines through a module hook.
   

   This comment needs some work, but I don't have a better way to specify that.

3. +++ b/modules/system/system.module
   @@ -2519,8 +2522,17 @@ function _system_rebuild_theme_data() {
   +  if ($module_themes = module_invoke_all('system_theme_engine_info')) {
   +    foreach ($module_themes as $name => $theme_engine_path) {
   

   Copy and paste is fine, but lets use $module_theme_engines instead of $module_themes here.

4. +++ b/modules/system/system.module
   @@ -2519,8 +2522,17 @@ function _system_rebuild_theme_data() {
   +      $engines[$name] = (object) array(
   +        'uri' => $theme_engine_path,
   +        'filename' => basename($theme_engine_path),
   +        'name' => $name,
   +      );
   

   I am not sure we want to allow to overwrite e.g. phptemplate with that.

   Maybe we want to use:

   $engines += array(
   $name => $theme_engine_info,
   );

   Not yet sure.

We do want to expose the functionality to modules _only_ for testing purposes or would it make sense to have a twig module, which then ships the twig engine without having to copy it to sites/all/themes/engines/twig?

I definitely agree that modules should be able to provide theme engine, really, copy/pasting it the worst thing ever. But I guess this should be another issue?

My 2 cents about your comments:

1. I guess it's related, since the engine needs to be found one way or another.Indeed, moving this out.
2. I think it should be left as a @todo wait for the issue #XXX (where X is the issue about providing the hook for everyone and document it) to remove this comment
3. Or just $engines or $engines_info, it really doesn't matter the name in the end, but you're right about $modules_themes not really being a good name.
4. The right place would be an alter hook, but it doesn't matter, people are not supposed to override phptemplate, but if they do want to do it, why not? As an initial version, I don't think is a blocking problem and could be refactored in a follow-up.

Don't mean to be rude, but seriously why did you wait to have 2 different issues to review? We are seriously loosing time for pretty much 0 benefits. Once we'll have fixed everything you said, how much time is going to take to fix the other patch, once again, then review, once again, then fix once again. I'd say this'll going to be commited in maybe, a year or so? Sad Drupal guidelines are sad, we are once again blocking this for pretty much nothing. I have a real job in the real life, I don't have Drupal dedicated hours, I have real bugs in there, and real projects rolling with those patches. It's not just frustrating, it's also a huge time loss for me.

Here is a new version of the patch.

So, what I did is:

1. Reverted this one-line change.
2. Removed the comment, since it's documented in system.api.php, we don't need to write this useless comment.
3. Changed to $module_engines, seems a good accurate name, right?
4. Just left it the same, I seriously doubt this will be used a lot, and it still lets people override the phptemplate engine, and this doesn't really bothers me.
5. And, bonus, moved the theme_test_system_theme_engine_info from #1545964: Do not copy over the owner and engine of a theme if the child theme uses a different engine than the base theme right there.

Changing the issue title, this will work for everyone not just testing modules.

Fails seems unrelated.

Done.

@Fabianx all your comments have been carefully considered and fixed, any other review to share or is this OK with you?

My pair of eyes just reviewed too

Loos very nice, lets please get the first part of the test in. (The basic test of the additional theme engine).

As already stated:

- Only the subtheme part belongs in the other issue. (The failing test + the changes to deal with the engine)
- We need the _basic_ other engine tests in here. (Those that don't need any more changes except what is already in this patch)

Small nits:

+++ b/modules/simpletest/tests/theme_test.module
@@ -31,6 +31,14 @@ function theme_test_system_theme_info() {
+ * Implements hook_system_theme_info().

hook_system_theme_engine_info().

is what you mean - I think.

-----

I really want to make this happen as much as you do, however I (and David and Stefan and all the D8 committers) are much more comfortable to get small and focused issue s in quickly.

But please do not forget that 100% of my D7 maintainer time is my private and fun time. I currently do not get sponsored for it. So lets please not be bitter about core ;).

Especially with the scope that e.g. the twig module can ship the engine in the module, it is important this is its own issue, but we need those tests.

The "context" I tried to set was:

- Only the bugfix parts belong in the other issue
- All the new subtheme issues and the hook (API addition) belong in here

Where did I fail to be clear enough? And how can I improve that?

Thanks!

Where did I fail to be clear enough? And how can I improve that?

It's just a matter of opinion, in the end, it's clear that we agree to disagree. I'm just saying, my opinion is pragmatic and what's matter the most to me is getting this in, not doing it properly, and doing everything at once in the other issue would just have been, very pragmatically, a lot faster and easier to do.

Because right now, no matter how hard you want to split this into issues to make it "cleaner" (once again, just a matte of taste and not an absolute truth) believe me or not, I strongly disagree with you when you say those don't belong together because the one issue actually tests the other, you cannot really dissociate them in the end.

But, as a side note, please also take into account that's I am really doing it, because it seems you won't ever listen to people actually writing this code, and I do respect your position and opinion. Doing it only to please you, and because you are blocking the whole fix. But the more you camp onto this position, the more it's becoming a frustration, and please, don't make me loose my fun too.

The more I am working with the tests, the more I find out that it's almost non doable to test the module provided theme engine without going through the other issues bugfixes.

Hum, by fixing our tests, I managed to find various bugs due to someone that wanted to be smarter than the machine, which is a definitive error: in drupal_find_theme_templates() for the only pleasure of not doing a foreach, some preg_grep() are in use [EDIT: it is actually to support the 'pattern' theme hook info feature, it took a while to realize that, I do have to apologize for the previous statement I made] forcing the original author to use drupal_system_listing() key lookups to find back template files. The whole algorithm is so wrong, because as a result of this "superbe optimization" (what the fuck seriously) [EDIT: it actually is a good optimization for theme functions discovery but not an optimization for theme templates, just the 'pattern' feature support] a theme engine may only use a 2-part extension (eg. ".tpl.php" or ".html.twig") but cannot use a single part extension (such as ".phtml"). No one ever saw that comming, because almost no one actually write theme engines, but in the end, this as another side effect: you cannot name your templates "some.thing" (for exemple, that would tie to the "some_thing" theme hook). Problem here is that actual core tests are using templates with the following name: "test_theme.test_template", this template contains "Failure: ..." as content, and it serves the purpose of testing that this template is NOT displayed. But because of the dual bug I exposed upper, the template was actually never ever found, so "Failure" was never ever displayed, so test was always a false positive.

Nice core is nice.

Please, Fabianx, I am solving all of this, but please, I am begin you, stop being a perfectionist and please be a pragmatic: when you force people to dig too deep under the carpet, you always end finding dirst. [EDIT: removed insulting comment, I do apologize for this]

Here is my final version of the patch, if it passes tests, just commit it and get over it. Once again, I had to debug so much time and fix so much bugs.

If you don't understand my comments in the code, just trust me and forget you read it, commit.

If you want to nitpick, I won't change anything more, I already lost too much time.

Green, feu dans le trou!

Looked at it, ok for me, no visible regression

I thank for the effort, however I would appreciate communication in advance instead of hours long coding and debugging - especially if you don't want to do it and need to be efficient with your time. Good job on finding that bug!

I am generally following the guidelines and the example of the Drupal 8 committers (And I well know the side of being the coder myself) :) (and the work that David did before obviously).

To the issue itself:

--

The template can simply be renamed and the problem is gone. Tests pass locally for me.

Here is the new patch and interdiff, also directly fixed two nits.

--

Looks good to me now, but one of you will have to re-RTBC (per the process rules).

Added a draft change record here:

https://www.drupal.org/node/2826480

You should keep my changes instead of reverting the whole. It does not induces any changes in the behavior, and makes the template name matching a bit more flexible, and I think it's the way it should go.

You remove all my comments, this is NOT ok, whereas I do agree with rewriting them to make them a bit more english and little less gibblish, but history must remain in comments, else people hacking or debugging this piece of code won't understand.

I'd prefer to keep the buggy theme template name, it's been here a long time, and that's an excellent use case to demonstrate that the new fix actually works.

By the way, your fix does not include the "dot separated 2 parts theme engine extension name" which is a real bug, and by fixing this also makes the theme engine definition more flexible, don't go back on this.

And please, just remove this infamous preg_grep() my foreach() simpler version is actually much more readable and easier to understand. [EDIT: found out, for the first time of my life, what the 'pattern' key actually really does in hook_theme()] I am doing Drupal for 10 years, and an awful lot more in coding in general, still I had to take 10 minutes to understand what it really does. Believe my experience: simple is better, don't go back on this.

I won't RTBC your version of the patch, I do accept the nitpicks, and will fix anything that needs fixing at this point, but don't change the patch please, theme system will be far better with the fixes I made than without.

Basically, my fixes, even if a bit too verbose are much less error-prone, instead of just pruning templates names using the first dot, I do just remove the awaited extension, which makes the whole template name parsing really work as expected, and much more robust. It is documented nowhere, I mean nowhere, that the number of dots in the engine extension is significant, and seriously, it should not be, my code does not only allows dot in template names, it also fixes that.

Basically, the original code Drupal 7 works on has serious flaws, and I did, even if it's a very small piece of the whole, made it more robust. Going backwards after the work done by me, @SebCorbin and @Pol at this point without any explaination nor review is a bit disrespectful.

I did a few benchmarks and research, it happens that the preg_grep() calls are way faster than iterating over an array, and that's probably the reason it's being used. It should be commented in the code, it's not, and that's a serious problem.

That said, on the theme template override use case I'm not sure this is significant, there will not be thousands of templates in themes. The benchmarks I did where iterating over 4000 function names, with 10 prefixes repeated 100 time (so 4,000,000 iterations) and the difference between preg_grep() and foreach was about 800ms (less than a second) for such huge amount of data.

The more I get into, the more I understand every choice, but because nobody documented it, I have to debug and benchmark every line of the way.

Here is an updated patch:

• restored the preg_grep() for supporting the 'pattern' feature;
• shorter and more focused comments;
• kept the whole change of filename handling, because this is a real bug, and it needs to be fixed, code is more robust this way;
• did not change the template name, actually I don't see why not supporting dots in template names, since that the theme engine extension matching is more robust, it works for no cost.

@Fabianx supporting the dot in template name is subject to discussion, since it's something core did not support and I'm OK to discuss that, aside of that the patch is ready for review once again.

But please, the fact is we need that to fix the other one, this needs to go in, and it would a good thing to have it before next 7.x official release, it would allow @Pol, myself and other theme engine developers to fix their modules once for all, and remove production site patches and .engine file linking or copy/pasting for a lot of people.

Forgot the patch :)

By the way, looking in Drupal 8 ode, the 'dot' wrong split for theme engine extension has already been fixed there (it fetches the complete filename from the file_scan_directory() and use str_replace(), which is different but pretty similar) so that's OK to say this patch doesn't need any Drupal 8 discussion at all since everything's already been fixed there. Drupal 8 second half of the function is actually closer to my original patch than it is from Drupal 7.

I added tests for the theme hook 'pattern' override.

In my opinion, this feature should be dropped, I never encountered any module actually using it, and core does not use it itself. But, because Drupal 7 is a stable product, I am aware that we cannot, because sites could actually use it, so I added the tests for it.

It also as a side effect tests that theme engine's _init() callback is run, through the theme_test_nyan_cat nyan theme, which actually a good thing.

Oups, sorry, I made the patch using the wrong commit number, here is the real one.

And finally a last one that changes the nyan_cat engine extension from nyan_cat.html to nyan.cat.html to add an extra dot in it, which validates that the other fixes are also working fine.

Failures seem unrelated, requeing it just to be sure.

They are related, once again I wrote [] for array syntax, fixing this.

Yup as expected, I wrongly used the PHP 5.4's [...] array syntax, once fixed in #42 it does work as expected.

Problem is my tests are testing both with the same 2 test classes: it does make sense to test an additional theme engine AND the fact it's well functioning at the same time, since the feature is about letting theme define theme engines, and the bug about the theme engines. It's not an easy one to split, just don't split it.

One thing that potentially may be removed from this patch is the test case about theme hook 'pattern' behaviour, but since it's only a single test, and the rest of the patch touches the code that handles it, I don't see why it should be split: I slightly modify the template discovery, hence the test about template discovery.

You know, it's just the 4th issue for the only goal of allowing theme inheritance with different engines, and the more we split it, the longer it will take. 4 is a lot, and now that we are here, let's just finish it.

The only way to make 2 distinct patches is to define yet another fake theme engine in core, which I hesitated to do, actually, if we don't, we'll have potential file name or other conflicts between the 2 patches. I don't think it worth the shot, but we can do it easily.

If D7 core tests weren't all in the same folder I would do it right away, but I was depressed just opening the folder, too much unrelated tests are stuck altogether and that's not very pleasing editing code in there.

What is wrong with just getting #27 in first, then doing any additional work in follow ups?

I am taking the liberty to re-upload #27. It is perfect in my eyes for the given scope of this issue.

If we need to test / fix theme patterns, I think we could also do that in a follow-up issue as well.

If any one of you is happy to RTBC #27 / the attached patch, I will speak with David and we can probably get that in quite soon.

Then we can do whatever else is needed in core to properly support twig.

Marking for commit, I only fixed some little nits and renamed a file. I will ask David to take a quick look, too - though.

#27 indeed solve the problem, it's just sad we have to open that many issues for this little set of changes. 10 years of this, most of my patches actually went to Drupal 8 and never end in Drupal 7, once the theme engine inheritance issue is commited, I will probably be done in helping fixing bugs in Drupal. Too much effort for no benefit, seriously there's no doubt why Drupal 8 developers did burnout, and there's no doubt why engineering is so hard with Drupal. Community is great, mostly driven by smart and nice people (really, I mean it) but processes and guidelines are bullshit, and you will continue loosing community members for this.

I mean, you never even told why it was a good thing to split, you never even tried to argue about why it needed to be splitted, and you even never reviewed fully the patch or answered any technical argument in all of this. If at least, there was a good explanation, with technical points, this would be a lot less frustrating and I would have the impression of speaking with someone that like and understand code.

And don't get me wrong, I know you are smart and you like code, I saw you do lots of smart patches. Problem is, first issue, you just said "let's split it" without any kind of technical review, nor even saying really why.

Please at least, be respectful for the work of others and consider it by saying something else than giving lessons to others on how to split issues. In that very way, Drupalism is the worst development process I ever known. It places the processes over the effort of people. Like if splitting this would actually change the course of action. It does not, it just frustrate people. It would have been more reasonable to keep one single issue at the very beginning, and ask others Drupal 7 maintainers to come and review 1 year ago when it was opened. Now is too late, and it's useless and fucking terrible for mental health. One year ago, it would have been revelant, now it's not anymore.

There's no mind to change, this was a wrong move from the first place, not technically I mean, but it's just a bad move for people's sake. Patch was fine in the other issue and we didn't need to do all of this, it's just the same work, but done 3 times.

Anyway I'm still glad see this patch ready. And yes it's Ready To Be Commited.

I mean, you never even told why it was a good thing to split, you never even tried to argue about why it needed to be splitted, and you even never reviewed fully the patch or answered any technical argument in all of this. If at least, there was a good explanation, with technical points, this would be a lot less frustrating and I would have the impression of speaking with someone that like and understand code.

Thank you! That explains what I have been missing.

The reason why I wanted the patches to be split is:

- a) This is a normal task and we want to also allow all modules to do so. It needs its own change record and is its own change. If there is any regressions or problems related to this, it will be easy to find and seeable.

- b) The original issue is a bugfix. To fix the bug we need a different theme engine, unfortunately modules could not ship theme engines, so this increased the complexity of the patch. It might have been okay to introduce the hook just for tests only, but that felt wrong.

The reason why I insisted (maybe wrongly) on splitting is that I felt you wanted to move quite fast and get this in earlier rather than later. (Unfortunately Stefan is not much available right now and I know David is pretty busy, too).

I tried to give a way that works 95% of the time to get core issues committed in a faster way (separation of concerns, as little changes as possible, as much as needed).

It would have been perfectly fine to tell me: I tried to split this, but due to the '.' in the theme template name, it can't be done.

I would have understood and said: "Okay, then lets see if we can avoid splitting this."

Or I would have looked at the problem and said:

"Okay, maybe we can rename the template. That is a known bug, but we don't need to fix this here."

Or I would have asked:

"Is there another way we can solve that?"

I can understand that both D7 and D8 development have been frustrating you, but the way to get issues into core is not to write more code and rewrite everything 90 times, but to write less code and communicate more.

Core committers are neither perfect nor is their judgement final nor is anything we say fixed in stone. Please talk with us, don't just run with the interpretation of what we say. (And I'll try to use a different less authorial tone next time, too.)

But please don't demand that we need to get this or that code in, just because you wrote it and think its perfect.

I can't do that and I explain why:

You might write it in the best interest, but if anything goes wrong (any regressions, broken sites or whatever), then I (and partially the other core committers) am responsible for that. We need to deal with the bug reports, we need to possibly revert the whole thing, we need to do damage control. There are around a million Drupal 7 sites right now. This is a huge responsibility.

Can you understand why we try / need to be conservative?

Have you ever tried to get code into Symfony/Twig? Or git? Or the Linux Kernel? Or other really big open source projects?

This is not just Drupal that likes small and focused changes.

I myself have *sighed* when I had to split a patch for D8 or write yet another test or do something else, but in the end often it was much worth it. (And I have experienced the same with Symfony/Twig and Git)

--

I also misinterpreted things. When I reverted your written code, I wanted to get the issue to a state where it can easily get in as I interpreted what you wrote that you would not spend any more time on it. So I thought: "Okay, then I'll just quickly get the issue to the state where it can go in, we RTBC it and make it ready for commit."

Lesson for me:

- "Explain better why I cannot accept random changes that have nothing to do with the issue at hand."

--

Overall pounard, you have a choice:

- You can continue what you have been doing to "fight" in the core queue and try to push your issues in with much much effort and blame the process and the core committers for not seeing the value of all the work you are doing. And eventually some will go in. (That all is my interpretation based on the issues I have seen. My interpretation might be totally wrong, though. So please take this just as another perspective.)

or

- You can simply ask: "What is the most effective way to get this in as quickly as possible?" and then we can talk and laugh about it and find a way.

Obviously you can do whatever you want, but I personally would very much appreciate to just talk about it.

And I myself do that a lot for D8. I just talk with Alex Pott or catch or Daniel or Crell or Wim or whoever else is responsible and say: "Hey I like to get this in, do you see problems with my approach?" It has worked out well so far.

I will also try to be more respectful of the already written code and ask more instead of giving orders (What have I been thinking, I might have had a bad day! Sorry for that.) :).

Okay?

This is a normal task and we want to also allow all modules to do so. It needs its own change record and is its own change. If there is any regressions or problems related to this, it will be easy to find and seeable

The original issue is a bugfix. To fix the bug we need a different theme engine, unfortunately modules could not ship theme engines, so this increased the complexity of the patch. It might have been okay to introduce the hook just for tests only, but that felt wrong.

It could have been another issue to just document it, as far as I known there's a lot of non-documented hooks and variables in core. In the end, it would have been as easy to just make the other patch pass with the hook, and finalize its documentation into another; it would not have been a Drupal way violation either.

It would have been perfectly fine to tell me: I tried to split this, but due to the '.' in the theme template name, it can't be done.
I would have understood and said: "Okay, then lets see if we can avoid splitting this."
Or I would have looked at the problem and said:
"Okay, maybe we can rename the template. That is a known bug, but we don't need to fix this here."
Or I would have asked:
"Is there another way we can solve that?"

You know, theme system is an ugly beast, and sometime it's better to fix multiple bugs in one shot that splitting into smaller issues. There is not much Drupal 7 core reviewers and commiters, and even less that actually do know how the theme system works by heart: the more you split, the more you remove context, and the less you give chance that everything will be reviewed.

When fixing multiple bugs at once, at least we're sure that people reviewing the stuff won't miss a single spot, and don't need to switch between multiple issues. The more you switch, the more it become difficult for your brain to switch, because, sad story, but the humain brain isn't really a multitasking kind of bitch. When you start reviewing a patch, if it's a single patch, you don't loose context between issues, and it's in end easier to do. Just saying that for your own sake.

You might write it in the best interest, but if anything goes wrong (any regressions, broken sites or whatever), then I (and partially the other core committers) am responsible for that. We need to deal with the bug reports, we need to possibly revert the whole thing, we need to do damage control. There are around a million Drupal 7 sites right now. This is a huge responsibility.

I do agree with that, and to be honest, I don't envy you. It's a great job to be a Drupal maintainer. But please, when I am writing the code, I am myself taking part of the responsability, and when it's dealing with code not a lot of people actually know, even more, just like you. My name would be in the deficient commit as well, and that's why I do rely on test when I can. And that's why I didn't want to split the issues once the tests were done, they were complete. I lost twice the time I originally had to debug just to rewrite the tests 4 times for every split or comment you made. Each time raising the probability of errors in my code.

Responsibility is a bitch, and sometime you need to trust people, especially when they're here for so many years.

- You can continue what you have been doing to "fight" in the core queue and try to push your issues in with much much effort and blame the process and the core committers for not seeing the value of all the work you are doing. And eventually some will go in. (That all is my interpretation based on the issues I have seen. My interpretation might be totally wrong, though. So please take this just as another perspective.)
or
- You can simply ask: "What is the most effective way to get this in as quickly as possible?" and then we can talk and laugh about it and find a way.

I didn't fight the first time, I raised against your opinion with technical arguments. So actually, conversation was not really possible because you didn't ask yourself. I will continue to fight stupid decisions, especially when they make loose time. 4 hours to debug and code to write the original patch, and I get the count right, 20 hours spent because of issue split.

You told it yourself, you're not many Drupal 7 core commiters, don't give yourself more work than you already have, and don't give US more work than we already did, that's not a fight, that's common sense against a purely logistical problem: you are not in numbers enough, you don't have enough time, and yet you asks us to do more work that'll be harder for you to review ? Wake up. Drupal issue queue really has unperformant processes and methods, if you don't have enough time for yourself, bend the rules, or anything won't ever really move.

That said, may we proceed with the patch please ? I don't want to abandon this one the same way I abandoned a huge lot of them because "split was needed" or there was "not enough tests" nor again "hum, let's wait for someone else's opinion". Because generally, the someone else finally appear one or two years after.

@pounard , mets-en, pour faire avancer les choses en 7.x j'ai dû affronter les mêmes barrages. Dés qu'on sort un release majeur en Drupal le focus devient la prochaine version et on met les bâtons dans les roues pour le progrès de la précédente et actuelle. Pour cette raison Quicksketch a créé Backdrop . Heureusement la direction chez Aquia ont posé quelques gestes pour apprivoiser ses politiques (un peu) , le fruit de cette souplesse est en version 7.50.

Étant donner que D7 sera probablement supporté d'ici jusqu'à 202? (je ne vois pas D9 arriver avant et je ne souhaite pas voir D9 avant 2035? , ceci est ma préférence).

On est en train de finalement réaliser le potentiel de D7, et on va faire beaucoup plus sans casser la compatibilité. Pour Noël je souhaite que Dries décide de libérer D7 des obligations envers D8. D8 est déjà sortie, on a déjà assez donné à D8.

Le montant de grande déficiences dans D7 qu'on a dû attendre 5 ans avant un commit est inacceptable. J'ai dû se battre tellement fort pour D7 que je n'ai pas beaucoup d'enthousiasme pour D8 parce que je sais que finalement D7 fonctionnent bien (mais je ne suis pas satisfait, on peut faire mieux et on devrait faire mieux et on VA faire mieux)

It might be a good idea to test a tests-only patch to ensure we are testing the right thing.

Assigning to me, no further action is required by participants of this issue.

There is no need for this to be postponed.

I reviewed the patch in #51 based on Fabian's request. It looks good to me (don't see any major issues) and I think it can go in.

The change record looks good too, although once this is committed we can change it to link directly to the hook documentation on api.drupal.org (once it appears there).

A few minor things that can be fixed on commit (well, at least the first two easily could):

+ * Tests the multi theme engine support.
+ */
+class ModuleProvidedThemeEngineTestCase extends DrupalWebTestCase {
....
+      'description' => 'Tests the multi theme engine support.',

I think both of the above should say "Tests module-provided theme engines".

+    'description' => "Serves a simple page renderered using a Nyan Cat theme engine template.",

Typo (should be "rendered").

+; Normally, themes may list CSS files like this, and if they exist in the theme
+; folder, then they get added to the page. If they have the same file name as a
+; module CSS file, then the theme's version overrides the module's version, so
+; that the module's version is not added to the page. Additionally, a theme may
+; have an entry like this one, without having the corresponding CSS file in the
+; theme's folder, and in this case, it just stops the module's version from
+; being loaded, and does not replace it with an alternate version. We have this
+; here in order for a test to ensure that this correctly prevents the module
+; version from being loaded, and that errors aren't caused by the lack of this
+; file within the theme folder.
+stylesheets[all][] = system.base.css

Why is this here? It looks like copy-paste from another test theme, but for this theme I don't think it's used in the tests at all. It can probably just be deleted...

Leaving this as is for now due to the above (and the tests-only patch Fabian wanted to do), but in general it looks like it's about ready. Thanks!

Heureusement la direction chez Aquia ont posé quelques gestes pour apprivoiser ses politiques (un peu) , le fruit de cette souplesse est en version 7.50.

For what it's worth, I'm not sure Acquia really had anything to do with that :)

David, about the copy/pasting it was intentional (at least from me) because what's said in there remains true, I figured out that, I as a developer, is glad to have stumbled upon this comment (it helps comprehension of the .info file). I'm not against removing it but I think there should be just something like // See foo.info for more information or something like it.

Many thanks for the review, glad to see this moving forward!

RTBC +1

I will commit this within the next 2-3 days. (including adding the tests-only patch quickly in advance)

Thanks, all!

I agree the code comment is useful if the stylesheets line needs to be there, but what I was trying to say is that I think the stylesheets line itself is not needed either. This theme is not used in that test.

The attached removes it; as long as tests still pass this should be good to go. I am adding the tests-only patch requested above while I'm at it also.

Thanks a lot David, you're probably right about the comment, it's useless if stylesheets are not in use. Patch is green!

Committed and pushed to 7.x! Thanks all!

Thanks a lot @Fabianx, and sorry for being a jerk sometime, there was nothing personal but just a lot of global Drupal-ish frustation, thanks again for commiting this.

hook_extension() in Drupal 7? o.O

https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Render!theme.api....
http://cgit.drupalcode.org/drupal/tree/modules/simpletest/tests/themes/e...

good observation @nevergone

I did a search through the entire 7.x core and only found one mention of hook_extension.

grep 'hook_extension' * -R
modules/simpletest/tests/themes/engines/nyan_cat/nyan_cat.engine: * Implements hook_extension().

Is this something that is going to be supported by contrib? or planned to add to core?

That's not a documented hook, only theme engines need to implement it for core template discovery to work. Drupal 8 documentation could be written for Drupal 7 the same way.

@pounard Where is associated module_invoke()/module_invoke_all() function in commit?

The hook_extension() is present in Drupal 7 already, but not documented.

I am fine to open a follow-up for adding the documentation to theme.api.php - similar to Drupal 8, but given that only 1% will ever implement custom theme engines, that feels not like a major problem to me.

@nevergone not all hooks are called using module_invoke*() some are actually called manually with function_exists() and call_user_func()

as per comment #81 by @Fabianx , is this the right place to put a follow-up issue for the documentation for theme.api.php?

#2831449: Document theme engine "hooks" (like hook_extension() and hook_render_template()) for Drupal 7

Drupal 7.53 wound up just being a small bugfix release (to fix a single regression from Drupal 7.51) so this issue was not included there. It will be in the next non-security release instead (which most likely will be labeled "Drupal 7.60" and have a few big changes in it).

I will update the change record to point to Drupal 7.60 rather than Drupal 7.53 also.

Thanks

David, could you please review #1545964: Do not copy over the owner and engine of a theme if the child theme uses a different engine than the base theme as well ?

Fixing tags since this is actually going into Drupal 7.54. I also updated the change notice to link to the documentation at https://api.drupal.org/api/drupal/modules!system!system.api.php/function... (see #65).