Problem

  • Contributed modules are not able to test their functionality, because they provide functionality for themes.

Goal

  • Allow contributed modules to test their functionality by allowing modules to register themes.

Details

  • Drupal core contains only a few tests that attempt to verify theme-related functionality. E.g., parsing of .info files, base/sub-theme detection and inheritance, and checking for update status.
  • To make those tests possible, Drupal core ships with a few dedicated and hidden testing themes in the /themes directory. Each of those themes exists for a particular test case.
  • Only Drupal core is able to put testing themes into a themes directory.
  • Contributed modules are not able to put a theme into one of the themes directories. Without a theme that implements the module's functionality, the functionality cannot be tested.
  • Modules need a way to ship with a testing theme that implements their functionality.
  • The proposed solution is simple: A new hook_system_theme_info() is added.
    • It is invoked in all enabled modules after Drupal has scanned the usual filesystem locations for themes, and before the collected themes are processed.
    • This allows testing modules to ship with additional testing themes, and to register and enable them when running tests.
  • The new hook may lead to the impression that the separate concepts of modules and themes are getting blurred and wrongly intermixed. However, this patch does not touch any of the fundamental aspects of themes. It merely allows an enabled module to tell the system to look into additional directories to find themes. It does not change any other logic or processing that is bound to themes.
  • Other implementation approaches were attempted, but failed very quickly, since they attempted to hack and squeeze in the additional theme information at runtime, not always providing the additional information to all theme info consumers, and got stale, obsolete, or lost upon clearing of statics and caches, etc.
  • A side benefit of this patch is that Drupal core itself is able to move the testing themes into the test directories of the corresponding modules that need them. Since Drupal automatically registers all themes it finds in the filesystem, this change slightly improves performance when themes are rebuilt.

Notes

  • This issue is not about testing a theme itself. It is about testing module functionality, which can be used/implemented by themes.
  • The testing theme files in the patch are merely moved/renamed without changes.

User interface changes

None

API changes

Addition only. No existing code will be affected.

Adds hook_system_theme_info() to system.module and system.api.php.

Original report by @sun

Modules like http://drupal.org/project/edge and http://drupal.org/project/skinr are not able to test major parts of their functionality, since the functionality requires a testing theme.

Only Drupal core is able to put testing themes into /themes/tests/*. Modules have no way to do that.

Attached patch allows modules (ideally hidden testing modules) to implement hook_system_theme_info() to return additional theme .info files outside of the regular 'themes' directories in a Drupal installation, so they get picked up during the rebuilding of themes.

--
Testing themes is the primary use-case only. There are further, secondary use-cases, such as #951212: Dashboard blocks should be configured completely independently from the default/admin theme blocks or http://drupal.org/project/admin. The idea for this solution actually comes from aforementioned issue #951212, in which I successfully used this hook already, so I know that it works.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

carlos8f’s picture

The idea is reasonable. The new hook itself looks clumsy:

+++ modules/system/system.api.php	26 Oct 2010 15:11:12 -0000
@@ -1764,6 +1764,31 @@ function hook_module_implements_alter(&$
+ * @return
+ *   An associative array whose keys are system names of themes and whose values
+ *   are file objects of type stdClass denoting the .info file of the theme,
+ *   containing the properties:
+ *   - uri: The full path to the .info file.
+ *   - filename: The file name of the .info file.
+ *   - name: The base name of the .info file, matching the array key.

I understand the hook is kind of edge-case, but it still doesn't make sense to have to return 'filename' and 'name', etc. if you're already giving the full path. Can we just have the hook return,

$themes['mymodule_test_theme'] = drupal_get_path('module', 'mymodule') . '/mymodule_test_theme/mymodule_test_theme.info';

and then derive 'filename' and 'name' from that?

Powered by Dreditor.

Jacine’s picture

Oh man, I didn't realize this wasn't possible.

Having the ability to do this would obviously be a huge help. I'm not sure I'd be inclined to go through the process of creating a bunch of tests without this, since they would not cover a large proportion of use cases.

sun’s picture

Thanks for the reviews! Attached patch simplifies the return value of the hook and moves the conversion into a file object to the caller.

carlos8f’s picture

Issue tags: +Needs tests

Looks good, let's try to add a test too. Especially because this hook might not end up being implemented by core (although #951212 is certainly a good case to use it).

sun’s picture

Issue tags: +Needs committer feedback

Before work on tests happens, I'd like to see a fundamental yay or nay from core maintainers.

moshe weitzman’s picture

Seems like a good solution for the problem. +1

David_Rothstein’s picture

+1 from me as well. This is a new hook, but it's very nicely targeted to fix the bug.

I'm not sure we need to write a specific test for this because we should be using this hook when testing other theme functionality in core. For example, after this patch is committed, I would assume that all the test themes currently stored in themes/tests could be moved out of there and associated with test modules instead (does not have to be part of this issue). That would be very nice because currently, all those test themes will get loaded from the database via list_themes() on every single non-cached page request on a real Drupal site. That wasteful behavior would not happen any more if they were added via a test module's hook implementation instead.

EvanDonovan’s picture

+1 - this would be a great cleanup for the theme system, and is very simply designed. Also, would definitely be a better way to do #951212: Dashboard blocks should be configured completely independently from the default/admin theme blocks, as comment #16 on that issue states.

sun’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Performance

Actually, even though I saw all of those testing themes when doing dsm()s for this patch, I totally didn't think of potential performance improvements David mentioned in #7. That's correct: We are needlessly loading, parsing, and preparing 3 hidden testing themes on each and every Drupal site. I suspect the performance impact is negligible, but nevertheless, tagging with Performance.

"Needs committer feedback" doesn't seem to work for non-RTBC issues, so bumping to RTBC in the hope to get some.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs committer feedback

It's more that "Needs committer feedback" doesn't work when said committer is on-site with a client and has no availability for anything but critical bug triage for two weeks...

It's a little redundant to essentially have the name of the .info file there twice (once as the key in the array, once as the actual file in the filepath), but I can't really think of a better thing to use as the key.

This looks basically okay, but it looks like theme_test module (or something) needs to implement this hook in order to register the test themes and gain the stated performance benefit?

dmitrig01’s picture

My module, wp_theme (http://drupal.org/project/wp_theme) would really, really, really, like to use this as well.

EvanDonovan’s picture

@sun: So it sounds like webchick is cool with this, as long as you add to the patch an implementation in the theme_test module....

sun’s picture

Issue tags: +API addition

.

sun’s picture

Status: Needs work » Needs review
FileSize
9.97 KB

So let's try and see what fails.

Status: Needs review » Needs work

The last submitted patch, drupal.system-theme-info.14.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
10.44 KB

D'oh. Wrong testing theme paths.

sun’s picture

Issue tags: -Needs tests

Passed! Hence, all of the existing Theme API and Update module tests are sufficient.

sun’s picture

Any other feedback? Or is this RTBC?

carlos8f’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. Moving /themes/tests to /modules/simpletest/tests/themes/ is a nice win. Let's just hope none of this stuff gets lost in the CVS commit :)

Note to @Dries/@webchick: we can safely remove the /themes/tests folder after this patch.

sun’s picture

Good call.

cvs add modules/simpletest/tests/themes/test_theme/template.php
cvs add modules/simpletest/tests/themes/test_theme/test_theme.info

cvs add modules/update/tests/themes/update_test_basetheme/update_test_basetheme.info
cvs add modules/update/tests/themes/update_test_subtheme/update_test_subtheme.info

cvs rm themes/tests/README.txt
cvs rm themes/tests/test_theme/template.php
cvs rm themes/tests/test_theme/test_theme.info
cvs rm themes/tests/update_test_basetheme/update_test_basetheme.info
cvs rm themes/tests/update_test_subtheme/update_test_subtheme.info
cvs rm themes/tests

That should cut it.

webchick’s picture

I think this makes sense, on the grounds of:

"Only Drupal core is able to put testing themes into /themes/tests/*. Modules have no way to do that."

It also keeps all of our testing code in the modules* or profiles* (which are basically modules in D7) directories, rather than messing with themes.

However, this definitely feels like the kind of issue that needs Dries's sign-off, since it really murkies up the distinction/relationship between modules/themes.

David_Rothstein’s picture

Belated review, but the patch (as well as the CVS spaghetti instructions in #20) look good to me also.

sun’s picture

Issue tags: -Performance, -API addition

#16: drupal.system-theme-info.16.patch queued for re-testing.

sun’s picture

#16: drupal.system-theme-info.16.patch queued for re-testing.

sun’s picture

#16: drupal.system-theme-info.16.patch queued for re-testing.

sun’s picture

#16: drupal.system-theme-info.16.patch queued for re-testing.

sun’s picture

#16: drupal.system-theme-info.16.patch queued for re-testing.

sun’s picture

Issue tags: +Performance, +API addition

#16: drupal.system-theme-info.16.patch queued for re-testing.

EvanDonovan’s picture

@webchick: You said in #21 that you were OK with this, I think, but wanted Dries to take a look? Is that still possible?

Kiphaas7’s picture

#16: drupal.system-theme-info.16.patch queued for re-testing.

bfroehle’s picture

~

sun’s picture

This should really get in prior to release.

webchick’s picture

I don't really understand why it would have to. It doesn't break anything API-wise, so could just as easily make it into 7.1, 7.2...

sun’s picture

#16: drupal.system-theme-info.16.patch queued for re-testing.

Jacine’s picture

It would really be nice to have this as soon as possible.

sun’s picture

At minimum the Skinr project really needs this patch now. A big part of the module's functionality is based around themes. It needs to test proper basetheme/subtheme inheritance (#1015614: Subtheme inheritance), theme hooks, and other stuff, which is impossible without this patch.

sun’s picture

Issue tags: -Performance, -API addition

#16: drupal.system-theme-info.16.patch queued for re-testing.

sun’s picture

Issue tags: +Performance, +API addition

#16: drupal.system-theme-info.16.patch queued for re-testing.

sun’s picture

If anyone could tell me what's missing or required to get this patch in, that would be great.

moonray’s picture

subscribe

effulgentsia’s picture

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

+1. We need to get it into D8 first, right? But please, let's do that, and follow up with a quick backport to D7 too. @Dries: see #21, webchick punted this to you.

sun’s picture

Issue tags: -Performance, -API addition

#16: drupal.system-theme-info.16.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Performance, +API addition

The last submitted patch, drupal.system-theme-info.16.patch, failed testing.

bfroehle’s picture

From @rfay in http://groups.drupal.org/node/141529

The testbots now use git to apply patches, so binary patches and copies/moves work in patches. See the issue.

To create a binary patch: git format-patch --binary --full-index --stdout

To create a moves-and-copies patch (one that magically moves and copies): git format-patch -C -M --stdout

Perhaps this patch be cleaned up as well...

sun’s picture

Status: Needs work » Needs review
FileSize
9.83 KB

oh dear, what a freakin' mess. Re-rolled against HEAD.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

catch’s picture

Issue tags: +Needs backport to D7

Tagging for backport.

sun’s picture

#45: drupal8.system-theme-info.45.patch queued for re-testing.

sun’s picture

sun’s picture

#45: drupal8.system-theme-info.45.patch queued for re-testing.

sun’s picture

We have

  • countless of reviews from core developers supporting this issue
  • countless of re-rolls
  • passing tests
  • a straightforward, backwards-compatible solution

for more than half a year here.

So pretty please, what's the effin' reason for not committing it?

Jacine’s picture

Yeah, I have to agree with @sun. Not having this patch in has negatively affected the progress of the 7.x version of Skinr quite a bit, so it would be great to see this committed and back-ported soon.

moonray’s picture

Agreed. Need this patch in ASAP.

pillarsdotnet’s picture

webchick’s picture

Something that would be helpful to know here is why the people +1ing this are +1ing it. It looks atm like apart from sun, no one has actually tested this patch, and are saying it looks good on visual inspection. Is there another use case out there apart from Skinr? If so, could more details be provided?

Not moving from RTBC, but some more details here would be great.

catch’s picture

That would be very nice because currently, all those test themes will get loaded from the database via list_themes() on every single non-cached page request on a real Drupal site. That wasteful behavior would not happen any more if they were added via a test module's hook implementation instead.

Just this from #7 is sufficient reason.

moonray’s picture

I've +1-ed this patch because I've been running it on Skinr patches for months now. It was already marked as RTBC, so I didn't figure I needed to add much more. I have yet to run into any issues with it.

Also, besides #7, #8 and #49 link to issues in core that could benefit form this patch. #7 in itself seems big since any resources saved to cut down on core testing time at this point would be beneficial.

catch’s picture

#7 isn't just testing time. All themes in the themes directory are loaded into memory every request on every Drupal install, because there is no real install status for themes, so this is an albeit small saving but one that will affect every full bootstrap for every D7 install.

Jacine’s picture

Something that would be helpful to know here is why the people +1ing this are +1ing it. It looks atm like apart from sun, no one has actually tested this patch, and are saying it looks good on visual inspection. Is there another use case out there apart from Skinr? If so, could more details be provided?

I'm +1ing this because we cannot run the unit tests we've written for Skinr without it. This means having the testbot enabled for our project is useless and that anyone who wants to help out and tests needs to hack core to do so. I've been running this patch since November of last year and haven't run into any problems with it at all.

Skinr is not the only use case here. Any module that needs to test its functionality in a "test theme" is screwed without this patch. @sun mentioned a few use cases in the OP, there's the Edge module, and @dmitrig01 mentioned he wants to use it for his wp_theme module in #11.

I would imagine there aren't many modules in this category that actually have unit tests yet, though. Almost everyone asking for this is a core developer, and there isn't a huge amount of contrib modules with unit tests so far. If we leave this, and make it impossible for these modules to write tests that actually work with the testbot, we're going to have a hard time getting people to write tests.

sun’s picture

Title: Testing theme-related functionality is impossible » Contrib modules are not able to test theme-related functionality

Summary:

Problem

  • Contributed modules are not able to test their functionality, because they provide functionality for themes.

Goal

  • Allow contributed modules to test their functionality by allowing modules to register themes.

Details

  • Drupal core contains only a few tests that attempt to verify theme-related functionality. E.g., parsing of .info files, base/sub-theme detection and inheritance, and checking for update status.
  • To make those tests possible, Drupal core ships with a few dedicated and hidden testing themes in the /themes directory. Each of those themes exists for a particular test case.
  • Only Drupal core is able to put testing themes into a themes directory.
  • Contributed modules are not able to put a theme into one of the themes directories. Without a theme that implements the module's functionality, the functionality cannot be tested.
  • Modules need a way to ship with a testing theme that implements their functionality.
  • The proposed solution is simple: A new hook_system_theme_info() is added.
    • It is invoked in all enabled modules after Drupal has scanned the usual filesystem locations for themes, and before the collected themes are processed.
    • This allows testing modules to ship with additional testing themes, and to register and enable them when running tests.
  • The new hook may lead to the impression that the separate concepts of modules and themes are getting blurred and wrongly intermixed. However, this patch does not touch any of the fundamental aspects of themes. It merely allows an enabled module to tell the system to look into additional directories to find themes. It does not change any other logic or processing that is bound to themes.
  • Other implementation approaches were attempted, but failed very quickly, since they attempted to hack and squeeze in the additional theme information at runtime, not always providing the additional information to all theme info consumers, and got stale, obsolete, or lost upon clearing of statics and caches, etc.
  • A side benefit of this patch is that Drupal core itself is able to move the testing themes into the test directories of the corresponding modules that need them. Since Drupal automatically registers all themes it finds in the filesystem, this change slightly improves performance when themes are rebuilt.

Notes

  • This issue is not about testing a theme itself. It is about testing module functionality, which can be used/implemented by themes.
  • The testing theme files in the patch are merely moved/renamed without changes.

HTH

Tor Arne Thune’s picture

Is there another use case out there apart from Skinr? If so, could more details be provided?

Comment #11: My module, wp_theme (http://drupal.org/project/wp_theme) would really, really, really, like to use this as well.

Something that would be helpful to know here is why the people +1ing this are +1ing it.

It seems like catch, moonray and Jacine has explained their +1 from previous posts. Comment #41 is really the only unexplained +1 now, as I see it.

webchick’s picture

Indeed; now if only one of those folks had given this issue a detailed tech review. :\

Anyway, Dries has been pinged again about this issue so hopefully he'll be able to get it to it in the next week or so.

catch’s picture

I've reviewed the patch several times but couldn't find much to nitpick, it looks like I never actually said that on this issue though.

Only two things stand out:

+function test_theme_theme_test_alter_alter(&$data) {

This is one of my very favourite hook names ever.

+ * This hook is invoked from _system_rebuild_theme_data()

This is another situation where we're adding a hook to a low level API function, and concerns me a bit in the same way that #534594: [meta] system info cache writing and registry rebuilding is broken without a full bootstrap concerns me.

However there are two reasons it doesn't concern me that much:

1. We already have two hooks like this (hook_system_info_alter() and hook_registry_files_alter()) - this isn't something really introduced by the patch.

2. Much more importantly, the theme system is considerably higher level than the module system and registry - i.e. you very rarely try to use a theme without modules being loaded, so in terms of extricating these circular dependencies it is probably the least of our worries. Also hook_system_info_alter() I'm guessing runs for themes already, so meh.

chx’s picture

I dunno, using a hook for this gives me the willies. Let's say you run drupal_system_listing('/^' . DRUPAL_PHP_FUNCTION_PATTERN . '\.info$/', 'modules', 'name', 0); and throw away (i think array_diff_key) those returned by drupal_system_listing('/^' . DRUPAL_PHP_FUNCTION_PATTERN . '\.module$/', 'modules', 'name', 0);. To me, this looks like we will get a list of info files that are under the modules search path but do not have modules belonging to them. (Yes, there are more modules than info files but that does not affect us right now) What those would be , I wonder. Maybe themes under the modules, hm?

Edit: note that this can not break anything because modules are discovered by the second expression and that's exactly what we do not take into consideration so there is no way we mistake a module for a theme.

Edit2: http://drupal.org/node/474684#comment-4473162 allows you to depend on modules. We need the modules list for that but we already need the modules list for what I described above. Win?

chx’s picture

Status: Reviewed & tested by the community » Needs work

I think this is CNW. Is there a "needs discussion" status :) ?

catch’s picture

Status: Needs work » Needs review

At least the memcache project used to ship with memcache.info but no memcache.module. Last release actually added a module but that's at least one example where there was a .info in the filesystem without a .module file accompanying it. Cnr seems fine for needs discussion.

sun’s picture

@chx: Thanks - while that's a nice idea,

  1. there's no handbook page or API documentation anywhere, neither in code nor on drupal.org, that enforces that every .info file has to be accompanied by a .module. Thus, a filesystem scan and diff like you propose would have a high chance of hitting random .info files on arbitrary sites in the wild. @catch even provided an example already.
  2. it would be an additional filesystem scan, slowing down drupal_flush_all_caches() rebuilds even more. Registering the themes explicitly has no performance impact.
  3. it would allow to put pure, actual themes into the modules directory and those would be picked up as well, which is definitely not the intention.
chx’s picture

@catch wrong it had a .module it was empty and it needed to have one -- @catch, @sun check _system_rebuild_module_data it scans for .module files. Without a .module it never recognizes your project. I know, I just hit this the weekend, check modulate 1.0 it had to have a 0 byte module file.

catch’s picture

@chx, no, it didn't. Look at the 6.x-1.4 tag of memcache and find memcache.module in there.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Well, go for it. As these themes depend on the modules , short of a hook we can have only ugly workarounds.

jct’s picture

Added issue summary

jct’s picture

Issue summary: View changes

Add issue summary

sun’s picture

Thanks. However, there already was a very concise summary in #60. I've replaced the new with that existing.

jct’s picture

Cool, thanks for fixing it, @sun. Trying our hand at an issue summary sprint here at DrupalCamp Wisconsin.

jct’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Title: Contrib modules are not able to test theme-related functionality » Contributed modules are not able to test theme-related functionality
Issue tags: +Testing system
webchick’s picture

Assigned: sun » Dries

Putting this into Dries's queue to review (sorry, sun).

sun’s picture

Re-rolled with file rename detection.

aquariumtap’s picture

I've tested and used this patch with Skinr, which is required for its automated tests.

@webchick, re: #55:

Is there another use case out there apart from Skinr? If so, could more details be provided?

The next version of Fusion will also require this patch to test some of its functionality. To repeat a distinction @sun made, this patch is not about testing the theme, it's about testing the module's interaction with a theme. We are building a module that provides theme functionality.

So, +1 :)

pillarsdotnet’s picture

For HTMLMail to have tests, this patch is needed. There have been several issues where template overrides located in the theme directory are not working as expected, and having tests would be really helpful in establishing, documenting, and ensuring proper behavior.

tstoeckler’s picture

Also, Color module currently tests itself by turning on Garland, and should really ship with a testing theme instead (follow-up issue, though).

stephthegeek’s picture

subscribing

sheena_d’s picture

subscribing

catch’s picture

catch’s picture

catch’s picture

I've opened #1292284: Require 'type' key in .info.yml files to discuss adding a type key to .info files, that'd let us pick up any kind of extension-type thing anywhere in the file system but also allow for .module-less module files.

I'm going to leave this in Dries' queue since it's been there for a while already.

I'd support committing this for now since it blocks contrib module testing, and in the realm of hacks we have to support testing (like hook_registry_files_alter()) it is a small one.

chx’s picture

Status: Reviewed & tested by the community » Needs work

The wording is , by far , not strong enough. This needs to emphasize never to use this feature for anything but testing. Chaos would prevail otherwise as people ship ordinary themes as modules.

sun’s picture

Status: Needs work » Needs review
FileSize
7.58 KB

Clarified the phpDoc to be for testing purposes only.

pillarsdotnet’s picture

It might be nice to include a pointer to the Mock Modules page which explains that Mock (hidden) modules (or themes), have hidden = TRUE in their .info files, and also explains that they're used for testing certain hooks:

Sometimes, hooks we want to test are not used in any of the core modules, or core modules do not cover all the use cases of these hooks.

sun’s picture

At the time you're writing tests you should already know about .info file properties and how to hide extensions via the hidden property.

pillarsdotnet’s picture

Sure; whatever. I vaguely remember hearing about hidden modules, but had to do several searches to find the docs on how to implement one.

Of course, we want to set the bar high enough that stupid people like me never hack core, and the tests I've written while in my ignorant unlearned state are probably unworthy and should be removed.

pillarsdotnet’s picture

Nevertheless...

Status: Needs review » Needs work

The last submitted patch, system-theme-info-100644-90.patch, failed testing.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
7.58 KB

Okay; obviously I broke something. Re-uploading the patch from #86.

pillarsdotnet’s picture

Now I see what I did wrong -- didn't check "git status" after applying the base patch, and hence missed the added directories.

Re-rolled.

sun’s picture

pillarsdotnet’s picture

Okay; fixed from the other end, then. Now at least I can search for "hidden modules" and find the right page:
http://drupal.org/node/302577/revisions/view/630432/1684272

catch’s picture

Status: Needs review » Needs work

Looks like the comment is identical to me "most commonly used".

pillarsdotnet’s picture

Status: Needs work » Needs review

@sun's patch is in two parts. You have to read all the way through to catch the comment change.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Well let it be but the next unnecessary git format-patch will be thrown back to CNW. Edit: make sure to use git apply for this patch.

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Performance, +API addition, +Needs backport to D7, +Testing system

The last submitted patch, drupal8.system-theme-info.86.patch, failed testing.

Dries’s picture

Assigned: Dries » Unassigned

I re-visited this patch and I believe it makes sense. It's "yet another weird hook" (YAWH) but it gets the job done.

It looks like the patch in #94 need to be re-rolled though. Once it passes again, it can be committed by either catch or myself.

Thus, I'm removing this issue from my queue as a decision has been made.

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
4.19 KB

OK then, rerolled.

Status: Reviewed & tested by the community » Needs work
Issue tags: -Performance, -API addition, -Needs backport to D7, -Testing system

The last submitted patch, 953336_102.patch, failed testing.

chx’s picture

Status: Needs work » Needs review

#102: 953336_102.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Performance, +API addition, +Needs backport to D7, +Testing system

The last submitted patch, 953336_102.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
7.31 KB
Tor Arne Thune’s picture

The new patch looks good to me. The difference is that it just includes a new test theme (together with the others as before) that was added after the previous patch was made.

Tor Arne Thune’s picture

Status: Needs review » Reviewed & tested by the community

Since it's just a re-roll, setting back to RTBC.

Dries’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs review

Committed to 8.x. Moving to 7.x for Angie to review.

rfay’s picture

Title: Contributed modules are not able to test theme-related functionality » [8.x broken] Contributed modules are not able to test theme-related functionality
Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

Looks to me like this one broke 8.x block tests: http://qa.drupal.org/8.x-status

xjm’s picture

Looks like the commit did not include the moved files: http://drupalcode.org/project/drupal.git/commit/cb4d787

git status

git add .

;-)

Tor Arne Thune’s picture

And a git rm core/themes/tests/README.txt is needed too.

EDIT: Or not... Didn't check the commit. I just assumed, and you know what they say about that ;)

Tor Arne Thune’s picture

Here's the D7 version of the patch committed to 8.x (#106). Attaching an interdiff between the D8 patch in #106 and the attached D7 version to show that the only changes are path changes. Also one file didn't exist in D7 (test_theme/theme_test.template_test.tpl.php).

Leaving without -D7 prefix so that the patch can be tested when the time comes to move this issue to 7.x-dev.

xjm’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review
FileSize
6.65 KB

Same as above, just re-uploading 'cause I tested the other against 8.x.

xjm’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

For real this time, back to 8.x/NW for #110. :) Sorry about that.

bfroehle’s picture

Status: Needs work » Needs review
FileSize
7.89 KB

Attached is a patch which, when applied to 8.x, fixes the bad commit in #109 (by adding the missing files). git apply --index is quite useful...

git checkout cb4d7878^
curl http://drupal.org/files/drupal8.system-theme-info.106.patch | git apply --index
git commit -m "."
git diff cb4d7878.. > drupal8.system-theme-info-fix-bad-commit-109.patch
xjm’s picture

Priority: Major » Critical

Patches can't be tested now, soooo...

webchick’s picture

Title: [8.x broken] Contributed modules are not able to test theme-related functionality » Contributed modules are not able to test theme-related functionality
Version: 8.x-dev » 7.x-dev

Ok, I think I committed and pushed that, so D8 should be fixed. If not please mark back and I'll check again later tonight. Thanks for the great tip in git apply --index. I am totally going to use that from now on. I also learned about git clean -df. :)

The patch for D7 is too late for 7.10 (tomorrow), but I can review for commit either later this week or next week. I've already reviewed it in the past though, so if it's good for Dries, I should be able to commit it more or less from there.

webchick’s picture

Priority: Critical » Major

Oops. And restoring status.

Status: Needs review » Needs work

The last submitted patch, drupal8.system-theme-info-fix-bad-commit-109.patch, failed testing.

xjm’s picture

Status: Needs work » Reviewed & tested by the community

RTBC for #113.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

I guess it was next week. ;)

The only thing that's slightly worrisome on D7 is the moving around of those themes, since people doing FTP uploads overriding old files will likely not remember to go back and remove the tests from the other directory. However, in testing locally that didn't seem to bring up any errors.

Soooo....!

Committed and pushed to 7.x. Hopefully! :)

Tor Arne Thune’s picture

Great! Another long-going major bug bites the dust. The commit looks good :)

Status: Fixed » Closed (fixed)
Issue tags: -Performance, -API addition, -Needs backport to D7, -Testing system

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

Anonymous’s picture

Issue summary: View changes

Clearer indication that it is an API addition only.