The config_test.module does not provide any test classes.
If it is disabled, the Drupal\config_test namespace is not loaded.

However, during tests, simpletest_classloader_register() adds all disabled modules as well, even if they don't provide tests.

Test modules that provide plugins are being loaded, but the module isn't enabled, so neither are its dependencies.

In Views we have a plugin that uses the entity type defined by config_test, and our plugin is being loaded and run without config_test, causing fatals.

simpletest_classloader_register() will have to be tweaked somehow to only load disabled modules that actually provide tests.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
1.36 KB

Here's a test to show what I mean. config.module has test classes and should be loaded, but config_test.module does not, and shouldn't.

tim.plunkett’s picture

FileSize
2.07 KB

Maybe something like this?

Status: Needs review » Needs work

The last submitted patch, classloader-1780396-2.patch, failed testing.

tim.plunkett’s picture

Well, that prevents test classes from use-ing plugins from disabled modules. Which is a problem.

Drupal\system\Tests\Plugin\PluginTestBase has use Drupal\plugin_test\Plugin\TestPluginManager

But it never enables plugin_test...

sun’s picture

First of all, it appears to me that the issue summary only describes the symptom, but not the actual cause and problem. I therefore have the impression that we're jumping onto workarounds to "solve" the symptom, but not the problem. ;)

In particular,

In Views we have a plugin that uses the entity type defined by config_test, and our plugin is being loaded and run without config_test, causing fatals.

I do not understand why the Views test does not enable config_test module if it uses its defined entity type?

Second,

that prevents test classes from use-ing plugins from disabled modules. Which is a problem.

why is that a problem, and why is it assumed to work in the first place? I don't see why it should be possible to actively use plugins or classes from modules that are not in the module list.

The only reason why Simpletest registers the namespaces of disabled modules is to be able to find their test case classes. This registration of namespaces is definitely not to be mistaken with allowing to actively use classes of modules that are not in the active module list within a test.

Third,

although its relevance depends on some clarifications for the above, #1774388: Unit Tests Remastered™ might be able to help, in case this is about unit tests. (This is why I'm talking about the active module list above, which doesn't necessarily mean installed modules.)

tim.plunkett’s picture

I do not understand why the Views test does not enable config_test module if it uses its defined entity type?

The views test module that need config_test has an explicit dependency. However, the plugin of the disabled views test module is loaded anyway, but only during test runs, because of that function.

I don't see why it should be possible to actively use plugins or classes from modules that are not in the module list.

It's not trying to use them in that sense, but when the plugin file is being parsed with annotations, the use statements are parsed, and fail if its not loaded.

chx’s picture

Um. http://api.drupal.org/api/drupal/core%21modules%21simpletest%21simpletes... this is nonsense? Why are we lumping disabled module namespaces into the classloader? If we want to find tests, then find them in the very dirs provided here and be done...?

sun’s picture

Status: Needs work » Needs review
FileSize
2.91 KB

This should cut it.

re #7: The test runner needs to register the module's namespace in order to find and instantiate the test case classes in the first place.

chx’s picture

The classloader is only necessary to be able to insantiate a class without manually loading it. As we need to iterate the files to find them, it's not a big deal imo to keep a list of them around (cached?). But if #8 works, great.

Status: Needs review » Needs work

The last submitted patch, drupal8.test-classloader.8.patch, failed testing.

tim.plunkett’s picture

I believe this is causing the issues I've seen with plugins from disabled modules being loaded.

sun’s picture

Assigned: Unassigned » sun
Status: Needs work » Postponed
FileSize
3.27 KB

Attached patch implements the identical pattern we're using for drupal_container() for consistency.

However, the failing tests are caused by plugin tests being unit tests, but they attempt to access and use plugins from the plugin_test module.

PluginTestBase needs to extend the new DrupalUnitTestBase instead of UnitTestBase, allowing it to load the code of plugin_test module.

Thus, this issue is blocked on #1774388: Unit Tests Remastered™

Berdir’s picture

While writing simpletest_classloader_register(), we discussed if we should just register the /Tests namespace or everything and agreed on registering everything because test code might load and reference other classes which would then break.

However, that should actually just happen when you have e.g. a class in there that implements an interface or extends another class that is not within the Tests namespace. This is nothing new, it already happens in 7.x when you e.g. have a mock implementation of an interface inside your .test files. So we could decide to stop doing that.

I don't think sun's implementation alone is enough to fix this bug. It's a change that makes sense and can fix certain bug, but this is a problem that also happens outside of a test run, unless I'm mistaken. So we need to change both simpletest_classloader_register() and TestBase.

Only registering modules which actually do have Tests probably is a bit tricky, because we either need to store that fact somewhere or do a check for that directory for every module. And it wouldn't fix the problem either, because a module with tests *and* a plugin would still break stuff. It could only be an additional improvement.

I guess if there's one thing to learn from this issue then it's: Do not enable simpletest on production. :)

Berdir’s picture

Status: Postponed » Needs review
FileSize
683 bytes

Just to try and see if this would work.

An actual fix would IMHO need both this and sun's patch.

Status: Needs review » Needs work

The last submitted patch, only-register-test-namespace-1780396-14.patch, failed testing.

donquixote’s picture

How does the plugin discovery work?
Why does it not check module enabled status?

donquixote’s picture

Test classes can have 4 types of dependencies:
1. Stuff that needs to be available when the file is parsed
2. Stuff that needs to be available when the static method is called.
3. Stuff that needs to be available when the class is instantiated.
4. Stuff that needs to be available when specific test methods are executed.

These dependencies are not identical with those of the module.

For 3., we have a mechanism to explicitly register this stuff.
For 2. and 4., the method itself can do a module_exists() or similar, so this shouldn't be a problem.

For 1. we don't really know.
Luckily, it is only classes and interfaces that we want to be available. We don't need stuff in the database, in the module folders, etc. (*)

Before #14, the approach seemed to be to register all module namespaces.
This is fragile. (**)
With #14, we only register the test classes. I support this.
It might still break, if one of those test classes depends on something in a disabled module.

I see two possible solutions:
a) Magically load classes from disabled modules, that are needed for a test class. But avoid making anything we don't need available to class_exists().
b) Let the file of the test class explicitly declare its dependencies for parsing the file.

More on this in the next post.

------------------

(*) Test classes only depend on classes and interfaces.
On the other hand, the availability of classes might depend on stuff that is run in the procedural part of a module.
E.g. a module might modify the way the class loader works, register additional namespaces, etc.

(**) Registering all module namespaces is fragile.
Even if we change the plugin discovery to check module enabled status, there can still be issues:
- some modules might be disabled because they contain junk that just crashes. E.g. if they are under active development. Or if a broken module is shipped with a non-broken one.
- some modules might conflict with each other, if classes from both are included.
- some modules might do class_exists() for some decision, and expect a module to be enabled if the class is available. (Plugin discovery is just one example)
- some classes might depend on stuff within the procedural files.
- some classes in disabled modules might depend on stuff within the database.

Maybe some of this can be avoided by good module design, but I don't feel very comfortable relying on that.

donquixote’s picture

So, now to the two possible solutions. (#17, (a) and (b))

Problem:
- A test class might need classes in disabled modules, while it is being parsed.
- We still want class_exists() to return FALSE for the majority of classes in disabled modules.

Solution (a): Magically include.

Magically load classes from disabled modules, that are needed for a test class.
But avoid making anything else available to class_exists().

// In the class loader (pseudocode)
function load_class($class) {
  $file = find_file($class);
  enable_disabled_modules_loading();
  // Disabled modules are temporarily available.
  require_once $file;
  disable_disabled_modules_loading();
}

Solution (b): Explicitly handle dependencies in test class file.

// In the test class file (pseudocode)
// "Whitelist" specific files to be available, even if the module is disabled.
// Drupal knows where \Drupal\disabled_module\A\B is to be found, so we don't need any further information.
drupal_register_class_even_if_module_is_disabled('\Drupal\disabled_module\A\B');
namespace Drupal\my_module\Tests;
class MyTest extends \Drupal\disabled_module\A\B {..}

OR

if (!module_exists('disabled_module')) {
  return;
}
class MyTest extends \Drupal\disabled_module\A\B {..}
Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Framework Initiative, -Testing system, -VDC, -Plugin system

#12: drupal8.test-classloader.12.patch queued for re-testing.

Berdir’s picture

Berdir’s picture

Berdir’s picture

The attached patch combined mine and sun's patch. Not sure why my patch is currently taking forever to test, there isn't a reason for that?

Status: Needs review » Needs work
Berdir’s picture

Assigned: sun » Berdir

I'm working on fixing those test failures, converting them to DrupalUnitTestBase and enabling the necessary modules seems to be working so far.

Will make it a bit slower but worrying about that is currently a bad joke, considering that we went from 34 minutes to almost an hour in the last week(s) :(

Berdir’s picture

Status: Needs work » Needs review

Hm, not sure about this.

Except of one test cases which was caused by a bug in my simpletest_classloader_register(), all of these tests don't actually need something from DrupalUnitTestBase. They only require that the module's PSR-0 namespace is registered.

Which can either be done using DrupalUnitTestBase,although I actually had to extend it (in a currently quite hackish way, not sure why it didn't just work) or by simply doing an explicit drupal_classloader_register(). I'm not sure what's better, I kinda like the explicit registration as it allows us to still use UnitTestBase.

The third option would be to move some of the used classed to lib/ although we probably want to explicitly verify that we can load module classes, e.g. in the plugin tests?

Also removed the hacks from EntityManager and changed __construct() to setUp() in some Router test classes that I had to touch anyway, no idea why they were doing that, I don't think they're supposed to?

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, use-test-classloader-1780396-25.patch, failed testing.

donquixote’s picture

Sorry for being ignorant, but could someone update me on what is the current consensus?

  1. Test classes of disabled modules should be available for autoload during test discovery.
  2. All classes of disabled modules should be available for autoload during test discovery.
  3. Test classes of disabled modules should be available for autoload during test discovery and while running those tests.
  4. All classes of disabled modules should be available for autoload during test discovery and while running those tests.

This should also go in the issue summary then.

Berdir’s picture

Test classes (or more correct, Drupal\$module\Tests namespaces) are available if simpletest.module is enabled. This is also true when running a test that has simpletest.module enabled but not otherwise. Simpletest makes them available when they are required by it. Which means test discovery but also running tests. When they're available, then they're available for everything and everyone.

Plugin discovery does not care about modules, it only cares about registered namespaces. To avoid overlaps between these two systems, we a) can only register the Drupal\$module\Tests namespace for disabled modules and b) must not implement plugins within the Tests namespace. That namespace must only be used for test classes.

That's what the patch is trying to do, but I probably messed some parts up. I'm not sure if that's the consensus, but that's the only way that I can currently see that works.

donquixote’s picture

We could also move tests into their own namespace, e.g.
Drupal\Tests\(module)
to represent that they are somewhat independent of the module itself.

Otherwise, if we stick with Drupal\(module)\Tests, then #29 does make perfect sense.

Berdir’s picture

Status: Needs work » Needs review
FileSize
17.57 KB

Ok, so I'm a bit confused.

Using the test classloader shows that we a) do not (early enough) register the psr-0 namespace of the new module, so what happens is this, for modules that have config entities:

- module_enable()
- config_install_default_config()
- $module_import_create()
- entity_create()
- entity_get_controller()

Which can't find the plugin definition and therefore returns an empty class string and kaboom.

and b) we don't unregister on disable, so EntityManager still finds the definition, no kaboom there, just a notice because the schema isn't defined.

What I don't understand is that this should currently cause all kinds of crazy errors on module enable in 8.x but it somehow... doesn't. So I think I am missing something somewhere. There are also some things that I don't understand yet, for example why DrupalUnitTest::setup() does enableModules($modules, FALSE) and therefore does not register the namespace either, even after the fix, for modules which are defined in static modules. So I had to move that to an enableModules() call, which already calls config_install_default_config(), so we could probably remove that line. Or register the namespace otherwise.

Let's see how the patch does.

sun’s picture

This patch rather indicates that the plugin system has to be corrected to not (seemingly randomly) assume that every registered namespace in the classloader is a possible plugin provider. The list of namespaces to check has to be injected. The job of a classloader is to load classes, not to be a registry for plugins.

I think that is the actual problem that needs to be fixed. Ideally re-starting in a fresh issue.

The various patches in here also exposed some other/further findings that are interesting in general. I'm too tired right now to comment on them, so just one thing that might provide some clues in the meantime: module_enable() calls into system_list_reset(), and quickly after, it calls into module_list(), which circles back into system_list(), and since that was reset, system_list() records the new module's filepath and registers its namespace. Same procedure for module_disable(), just negated. Lastly, keep in mind that *UnitTestBase operates on a fixed/locked module list, which means that module_list() does not call into system_list().

tim.plunkett’s picture

@sun, have you looked at AnnotatedClassDiscovery::getDefinitions() ? Sounds like that's what you'd like to fix.

Status: Needs review » Needs work

The last submitted patch, manage-classloader-on-module-enable-1780396-31.patch, failed testing.

Berdir’s picture

@sun: Most of this patch is about actually making these classes available, limiting it could only help with the uninstall-part of this problem. Many of these plugin tests don't even try to enable the modules they are using.

Unless you don't want to make the classloader changes for tests/disabled modules which are included in this patch, which I don't think would be a good idea.

module_enable() calls into system_list_reset(), and quickly after, it calls into module_list(), which circles back into system_list(), and since that was reset, system_list() records the new module's filepath and registers its namespace. Same procedure for module_disable(), just negated. Lastly, keep in mind that *UnitTestBase operates on a fixed/locked module list, which means that module_list() does not call into system_list().

Right. Many of these tests are WebTests which enable modules with drupalPost() and they have problems too. Also, for module_disable(), this is exactly what I'm relying on. module_list()/system() list can only add a module to the classloader, not remove them, so what I'm doing is resetting the classloader so that we have a new one to which then only the modules which are still enabled are re-added.

Edit: Actually, now that I think about it, web tests only seem to have problems with disabling entity modules or if they didn't even try to enable the necessary modules (e.g. the views join plugin test ). Enabling seems to work. So you might have a point there. The question is then who's responsible for registering the namespace, does that mean we should do it in DrupalUnitTestBase in enableModules(), before calling module_enable()? That might be consistent with also injecting it into module_list(). Still not sure why DrupalUnitTestBase doesn't module_enable() modules defined in static $modules, can you explain that?

Berdir’s picture

Status: Needs work » Needs review
FileSize
3.89 KB
16.11 KB

The attached patch moves the PSR-0 registration from module_enable() to DrupalUnitTestBase::enableModules(). This allows to revert the changes in the config tests.

Haven' figured out those failing breakpoint tests yet.

Status: Needs review » Needs work

The last submitted patch, classloader-disabled-1780396-36.patch, failed testing.

Berdir’s picture

Assigned: Berdir » Unassigned

Have been trying to debug this a bit more. Somehow the module namespaces aren't registered again when calling module_disable() within that test. Removing the classloader-reset fixes that of course, but would then again break the other tests.

Unassigning for the moment, maybe someone wants to have a look until I have time again, probably not happening tomorrow.

neclimdul’s picture

edit: comment made not sense and deleted.

Why would the plugins system not assume the registered namespaces to be valid? It shouldn't be its responsibility to resolve arbitrary inconsistencies caused by simpletest.

sun’s picture

Why would the plugin system assume that in the first place?

The registered namespaces in the classloader are registered namespaces. Only. There can be multiple paths per namespace, there can be multiple registered namespaces per component. Where is the relation to a plugin registry?

Just take this example, which only happens to be one of a multitude of possible cases:

 function simpletest_classloader_register() {
...
+    $loader->registerNamespace('Drupal\\' . $name . '\Tests', DRUPAL_ROOT . '/' . dirname($data->filename) . '/lib');

Is Drupal\whatever\Tests a valid provider for plugins? — No.

Meanwhile, that clearly seems to be the root cause of the problem.

Berdir’s picture

@sun: IMHO it's the other way round: The fact that simpletest did that (register everything) is the reason why it currently *works* at all (combined with the hacks that were necessary in EntityManger).

Removing + having a separate classloader within the test shows the problems but isn't causing them. The root cause in most cases are the incorrect test implementations + some crazy runtime stuff during module_disable() (enable too, but disable is the fun one), something that the classloader currently isn't really designed to handle, because it's not something that you would ever do in a "normal" php framework those components are coming from: unloading code at runtime.

One way to fix that would be to subclass the classloader and provide a removeNamespace(). I don't really have an idea how it would work with ApcClassLoader, on the other hand.

sun’s picture

FYI: #1830170: module_list() doesn't register namespaces with the classloader when given a fixed module list. fixes the classloader problem with a fixed module list, which is a very focused change that is definitely correct.

donquixote’s picture

You can unregister a namespace from the class loader.
You can not undefine a class once loaded.

This will make any class_exists() after unregistering unpredictable.

attiks’s picture

#36 regarding the failing breakpoint test, I think it's ok to remove it, if it helps moving this issue forward.

Berdir’s picture

Hm, so #1830170: module_list() doesn't register namespaces with the classloader when given a fixed module list. actually caused a problem with this patch, because the installer currently enables the user.module, which makes it's namespace available too but then we can't load the schema of it as we're still in early install phase. Not sure yet how to fix that.

@attiks: That's nice, but I don't think an option :) We need to understand and fix it or we'll run into it again.

attiks’s picture

@Berdir, it would be nice to have, but what the test inbreakpoint is testing, is the fact the config removes files when the module is uninstalled, this can be a test on the config it self, because it is needed by everything that's using config.

sun’s picture

Thankfully, others independently came to the identical conclusion as I did in #32:
#1836008: Remove drupal_classloader() use in Drupal\Core\AnnotatedClassDiscovery

chx’s picture

I clearly do not get this. a) The issue title is wrong. No modules are loaded. Their namespaces are registered, yes but the modules are not loaded. b) if we are doing this the registration of namespaces just so that we can read getInfo then nuke getInfo and use annotations and... be done?

sun’s picture

Title: Disabled modules that do not provide tests are still loaded during test runs » Namespaces of disabled modules are registered during test runs

Yes, the issue title is wrong.

And no, we must register all namespaces, because you wouldn't be able to write any test, nor any integration test otherwise.

It starts with:

use Drupal\simpletest\*Base;

And ends with:

namespace Drupal\rdf\Tests;

use Drupal\comment\Tests\CommentTestBase;

class CommentAttributesTest extends CommentTestBase

Lastly, tests must be executable for disabled modules in the first place. And we also must not assume any module to be enabled in a parent-site/test-runner, nor that there is such an environment to begin with (because it is the root cause for the never-ending environment leakage problems we're facing and poorly "fixing" every other week).

I don't see the point in making test authoring considerably harder, when the actual bug we're facing is clearly identified with #1836008: Remove drupal_classloader() use in Drupal\Core\AnnotatedClassDiscovery.

chx’s picture

a) when actually running the test for a given module then enabling said module in the test environment, regardless of the parent, is a given b) if your module is using the testbase of another module (does that ever happen?) then it need to depend on it. I still think this would be a viable route but won't harp on it.

sun’s picture

a) is utterly wrong. Unit tests are not even able to enable modules to begin with.

b) circles back into a) and the bogus assumption that anything would be enabled at any time.

sun’s picture

Status: Needs work » Closed (duplicate)

Actually, let's mark this a duplicate of #1836008: Remove drupal_classloader() use in Drupal\Core\AnnotatedClassDiscovery

The actual bug is that the classloader's registered namespaces are abused for something they are not designed for.

The corrected issue title clarifies what is happening for tests, and this is by design.

EclipseGc’s picture

Status: Closed (duplicate) » Needs work

I actually posted why this is NOT a duplicate over in http://drupal.org/node/1836008#comment-6719086. The notion of reintroducing a registry has been brought up over there, and I think there are a lot of really good reasons to not revisit that fiasco, but at the same time, a registry for JUST tests, might be a good solution if no other good solution is forthcoming.

Eclipse

Berdir’s picture

Here's an attempt in listing what problems this patch is trying to solve and how. Trying to get an understanding of what exactly the other issue can and will fix.

- Simpletest registers all namespaces of all modules, including disabled ones. The current patch limits this to the Tests/ sub-namespace. Note that it doesn't matter if we use annotations or getInfo(), both approaches require the classes to be within the registered namespace. I'm not 100% how the other issue will affect this, but AFAIK, the explicit list can only be a sub-list of all registered namespaces I think?

- There is only one single global classloader for the global site and the test environment within it. This is solved by initializing a separate classloader during the testrun.

These are the two actual changes in this patch, everything else is a work in progress to fix the resulting test failures.

- Tests that rely on plugins or other namespace-related things provided by disabled modules. This is conceptually wrong and should be fixed. I'm not yet sure if that should be done by making those unit tests DrupalUnitTests or registering the required namespaces explicitly and manually in setUp().

- Namespaces aren't correctly updated on module enable/disable. This I think is the main thing that the other issue would fix. The main problem for enable is that e.g. the default config copying invokes hook which in turn except entity type info to exist, so we need to register it explicitly, *before* it would happen through system_list_reset(). Or we need to move that call up, but we'd have to repeat it for every module. Why disable is a problem is relatively obvious, this is currently fixed by dropping the classloader and letting the next call to system_list() re-add everything that's still exists. Being able to actually remove something from a list would be much nicer.

- #1830170: module_list() doesn't register namespaces with the classloader when given a fixed module list. then introduced an interesting problem because it also registers the namespaces of fixed lists of enabled modules. This caused a problem in the installer, which registers the user.module *very* early on and then it resulted in a warning because the corresponding schema couldn't be found. Looks like this might have been "fixed" by removing user from that list in #782672: Loosen the coupling between the user module and the installer/maintenance theme, yay!

I'm not sure at this point if the other issue will fix everything found here, it will at least need some of the fixes being worked on here and some concepts introduced here, like the need for a separate classloader/plugin-location-registry during test runs. Maybe postpone this on the other issue and re-check once the other issue is implemented to see if there's something left?

sun’s picture

I still stand by my earlier statements. The way we write tests will change at some point, but it's a very long way towards that, and the suggested change in behavior of registering namespaces would cause major problems for unit/integration tests, especially in contributed modules. Contrib is a dimension we commonly forget in core, and that dimension will also have to be addressed first, before we will switch to a completely different way of authoring tests.

Contrary to that, the latter two points in #54 appear more valid to me, but they are independent problems, and thus I'd appreciate it if we could split them out into 1-2 separate issues, instead of cramming them into this one. :)

xjm’s picture

I wonder if this is what's causing the horror in #1806334: Replace the node listing at /node with a view?

webchick’s picture

Based on the workaround that was required at #1875098: Edit's processed text editor plugins bleed into test runs to get Edit module's plugins to not show up for other modules when the module's disabled, I'm half-tempted to raise this to critical. This is not what any developer who wasn't embroiled in the guts of the plugin system for months would know how to do, and the default behaviour is exactly the opposite of what you would expect.

effulgentsia’s picture

I haven't kept up on the full discussion here to know if there's any problem here other than plugin discovery within disabled modules. If it's only that, then this could get fixed by #1836008: Remove drupal_classloader() use in Drupal\Core\AnnotatedClassDiscovery. Although it's still failing tests, human feedback on the latest patch there would be helpful.

Berdir’s picture

@effulgentsia: The fact that the plugin discovery test fails with your patch (an actual fail, not the exception) does in fact indicate that it might solve this issue.

I assume that you will have to convert those tests to DrupalUnitTestBase and enable plugin_test so that its plugins are found.

I suggest we finish the other issue and then remove those checks here and see what happens. I suspect that there might still be an issue or two in module enable/disable cases, which I was fighting with here, although it might be easier to fix them.

Also not yet sure what exactly we should do to the classloader during test runs...

Berdir’s picture

Status: Needs work » Closed (duplicate)

Went through this again and I think this can be closed now.

- Plugin discovery is now limited to an explicit list thanks to #1836008: Remove drupal_classloader() use in Drupal\Core\AnnotatedClassDiscovery
- Namespaces are now managed and updated within the Kernel
- Unregistering of namespaces no longer needs to happen
- The only thing that's left from the patches here is limiting the namespace registration of disabled modules to the \Drupal\$module\Test but that's not a bug and could be discussed in a new issue if we want to.

Wim Leers’s picture

Status: Closed (duplicate) » Active

I don't think this can be closed until work-arounds for this problem (e.g. #1875098: Edit's processed text editor plugins bleed into test runs, http://drupalcode.org/project/drupal.git/blob/780b4392973ad76eb40c65bef8... in the codebase) are removed? I count 6 hits when searching for "http://drupal.org/node/1780396" (i.e. this issue).

Berdir’s picture

Have you done a git pull before you did that count? :)

The referenced issue above removed them.

neclimdul’s picture

Priority: Major » Normal

As I've said all along, we have defined simpletest's job to be providing as close to a clean and consistent state as possible. If it does no do this, we can not rely on the code we're testing to behave in the same manner it will when its actually being used, which actually invalidates our tests.

We need to fix it regardless of whether we're currently seeing a problem because it is possible its masking flaws in our current or future tests and/or will cause unexpected problems in the future when some code makes a reasonable assumption about the state of the system.

Being as its not currently breaking anything I'll knock the priority down but we should definitely still fix this.

Wim Leers’s picture

#62: unfortunately,it still exists for Drupal\edit\Plugin\EditorManager I guess that was somehow forgotten, even though it's been there since January… All other references were indeed removed.

Berdir’s picture

Yes, noticed that as well. I'm removing that part in #1903346: Establish a new DefaultPluginManager to encapsulate best practices :)

EDIT: Not removing the class completely, only other ones.

Wim Leers’s picture

#65: that's now no longer part of #1903346: Establish a new DefaultPluginManager to encapsulate best practices, but it's currently RTBC. So let's remove this stray one here? :)

Wim Leers’s picture

Status: Active » Closed (duplicate)

Here's a patch to remove the remaining processDefinition in edit.module's plugin manager. It also removes the 'module' annotation keys from (edit|editor|ckeditor).module's plugins: #2006278: Remove leftover 'module' annotation keys and plugin manager processDefinition() to deal with disabled modules during tests. As per #64, #66 and a quick IRC chat with Berdir.

Restoring to state in #60.

effulgentsia’s picture

Status: Closed (duplicate) » Active

I think #63 still stands. #60.4 says a new issue can be opened for that, but until it is, setting this one back to active.

Wim Leers’s picture

Oh, sorry. Pinged neclimdul for that.

effulgentsia’s picture