Drupal's current goal is to provide a content management framework. However, core could easily do without Node.module, which would make Drupal a more general framework, especially now Block's been made optional as well. This would make Drupal's actual core (the group of required modules) far lighter and a better choice for apps that don't need content management, for instance. Another argument: does core need Node.module to function? IMO it shouldn't. User management is required, since you need users to configure Drupal. Filter.module is needed for security reasons and System is the core's core. Node.module adds content management. Do User, System or Filter require it? No.

CommentFileSizeAuthor
#145 drupal8.node-optional.145.patch25.05 KBsun
#143 375397.patch26.07 KBRobLoach
#141 375397.patch25.09 KBRobLoach
#138 drupal8.node-optional.138.patch23 KBsun
#133 drupal8.node-optional.133.patch22.19 KBsun
#127 drupal8.node-optional.127.patch20.87 KBsun
#117 drupal8-node-optional.117.patch20.06 KBXano
#116 drupal8-node-optional.116.patch161.54 KBXano
#108 drupal8.node-optional.108.patch22.13 KBsun
#106 drupal8.node-optional.106.patch20.23 KBsun
#101 drupal8.node-optional.101.patch19.58 KBRobLoach
#100 drupal8.node-optional.100.patch17.23 KBsun
#99 drupal8.node-optional.99.patch17.52 KBsun
#93 drupal8.node-optional.93.patch17.33 KBsun
#88 drupal8.node-optional.88.patch12.95 KBsun
#87 drupal8.node-optional.87.patch12.78 KBsun
#85 drupal8.node-optional.85.patch9.99 KBsun
#83 drupal8.node-optional.83.patch9.4 KBsun
#82 drupal8.node-optional.82.patch7.65 KBsun
#81 drupal8.node-optional.81.patch7.65 KBsun
#79 drupal8.node-optional.79.patch7.58 KBsun
#78 drupal8.node-optional.78.patch6.46 KBsun
#75 drupal8.node-optional.75.patch4.85 KBsun
#63 drupal.node-required.63.patch14.75 KBsun
#61 drupal.node-required.61.patch16.77 KBsun
#57 head.node-required.57.patch15.49 KBsun
#53 head.node-required.53.patch14.64 KBsun
#51 head.node-required.patch3.36 KBsun
#43 make_node_optional_14.patch56.69 KBXano
#40 make_node_optional_12.patch17.24 KBXano
#25 make_node_optional_11.patch17.98 KBXano
#23 make_node_optional_10.patch26.13 KBXano
#21 make_node_optional_09.patch26.07 KBXano
#20 make_node_optional_08.patch17.67 KBXano
#18 make_node_optional_07.patch17.64 KBXano
#17 make_node_optional_06.patch17.11 KBXano
#15 make_node_optional_05.patch17.06 KBXano
#11 make_node_optional_04.patch12.98 KBXano
#8 make_node_optional_03.patch12.41 KBXano
#7 make_node_optional_02.patch12.31 KBXano
#5 make_node_optional_01.patch11.97 KBXano
#1 make_node_optional_00.patch8.89 KBXano
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Xano’s picture

Assigned: Unassigned » Xano
Status: Active » Needs review
FileSize
8.89 KB

This is an initial patch I've been working on this evening. We intend to update Taxonomy so it uses Field and AFAIK it doesn't require the hardcoded relationship with Node anymore that is has now. If we pull that off, this patch doesn't have to worry about making Taxonomy independent from Node anymore.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Even without decoupling taxonomy from node, it could always have node as a dependency until that's done.

Xano’s picture

What do we do about the default front page? This is set to node, which will not be possible anymore with this patch.

Also, should we set Node as a dependency for modules like Blog, Forum, etc? They don't rely on Node's API, but are of no use without it. Therefore it's not strictly necessary to set dependencies, but it may very well prevent confused users.

Xano’s picture

Status: Needs work » Needs review
FileSize
11.97 KB

Slightly improved patch. SimpleTest often cannot perform the tests at all or returns PHP notices and warnings, which I haven't yet been able to solve.

Status: Needs review » Needs work

The last submitted patch failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
12.31 KB

Simplified the code. The patch also prevents a redudant menu rebuild at admin/build/modules. One of the tests still fails, I'll check that out ASAP.

Xano’s picture

FileSize
12.41 KB

All tests pass now. However, I'd like to get rid of the call to menu_rebuild() in system_modules(), which is now necessary for MenuIncTestCase->testMenuName() to pass. Simply replacing the drupalGet() call to /admin/build/modules with menu_rebuild() didn't work.

catch’s picture

Status: Needs review » Needs work

I've seen people complaining about node module being required in #drupal before.

However - it should still be enabled in the 'expert' profile - same as we enable stuff like dblog, block and menu in there to avoid a completely unusable (as opposed to just stripped down) site on install.

Things like moving hard coded stuff from system.module to hook_modules_enabled() are good cleanup regardless of whether node.module is optional or not, so I'd support those changes in their own right too.

Xano’s picture

IMO the Expert profile should only enable the absolutely necessary modules and Block, because the interface is nothing without a module that displays menus.

Xano’s picture

Status: Needs work » Needs review
FileSize
12.98 KB

Discussed this with catch and we agreed that change belongs in a different issue. This patch updates the Expert install profile.

tstoeckler’s picture

Status: Needs review » Needs work

When the patch is applied, and node module disabled, the home page of the site, gives you a "Page not found" error. Actually, I don't know how to solve that problem, maybe just make a static page, like the welcome screen after install, that doesn't depend on the node system.
I set this "needs work" because of this, if this issue is neglectable or should be part of a follow-up issue, set back to NR or RTBC.

One other note: node module currently does not let itself be unistalled. Would that also be part of a followup issue?

Xano’s picture

The problem with the front page is a known problem (See comment #4). Node uninstallation should be part of this patch as well. Just haven't got around to improving the patch yet.

tstoeckler’s picture

Sorry...

Xano’s picture

Status: Needs work » Needs review
FileSize
17.06 KB

Moved all the (un)installation code from system.install to node.install. Functionality seems to work. Let's see what our bot says.

Status: Needs review » Needs work

The last submitted patch failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
17.11 KB

Defining Node as the first module to be installed in the default profile fixed the problem. It works as far as I can see, but it seems there still is some sort of a dependency that hasn't been dealt with yet.

Xano’s picture

FileSize
17.64 KB

Install profiles don't pay attention to dependencies defined in info files, it seems, so the fix in #17 was no workaround after all. This patch introduces a Node dependency for Comment

Status: Needs review » Needs work

The last submitted patch failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
17.67 KB

My editor made me omit a comma.

We still need to decide if we're going to do something about the default front page, which is automatically being set to 'node'. This will not be a problem with the current installation profiles, since both enable Node. The point is that System module (I believe) uses 'node' as the default value and therefore is still dependent on Node in some way.

Xano’s picture

FileSize
26.07 KB

The attached patch moves the front page help you see if no nodes have been promoted to common.inc and menu.inc. Node now removes it's variables from $conf and cleares the variables cache at uninstallation as well.

(Drupal.org somehow manages to post a comment with the patch, but without the body text I just typed :S )

Status: Needs review » Needs work

The last submitted patch failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
26.13 KB

Adding an omitted drupal_set_title() prevents a lot of errors.

Status: Needs review » Needs work

The last submitted patch failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
17.98 KB

The frontpage issue is moved to #419950: Remove /node as default front page.

tstoeckler’s picture

Still works perfectly, including uninstall (It's kind of scary though to see your database without the node table...).
One thing I noticed, but that's probably not related / not in the scope of this issue: After disabling node module, node actions are still available. Same goes for comment actions though...

Xano’s picture

Status: Needs review » Needs work

Good observation! They shouldn't be available at all after uninstallation.

Dave Reid’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

Xano’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

Jaza’s picture

This is a great idea - node module is unnecessary if you're using Drupal for a completely custom web app that has no need for traditional content management. In fact, you may have forgotten it, but node module has been optional before in Drupal's history. It was optional in 4.6 and previous versions, and was only made required in 4.7. IMHO making it required was a mistake, and I think this is a great opportunity to undo that mistake.

re: #20 (the default front page being 'node'), I don't think this is an issue. Core only ships with 2 install profiles, and both of them enable node.module by default.

If you're advanced enough to be writing a custom install profile that disables node module, then I think it's reasonable to expect that you would know to change the default front page to something else (also using the install profile). However, we should add something to this effect in the 'how to write an install profile' docs.

Similarly, if you're advanced enough to be disabling node.module post-install, then you should know to re-configure the front page on the 'site information' page. Although it wouldn't hurt to add a drupal_set_message() call to hook_uninstall() in node module, telling users that they might need to re-configure their front page setting.

Xano’s picture

The problem is that currently the help text shown right after installation is part of Node.module. Also, we should make sure no errors are thrown on the front page if there is no Node.module. Core is responsible for this. The front page issue is being dealt with in #126221: Make 'node' empty text themable.. In this issue we focus on everything else related to making Node optional.

Xano’s picture

Note: remove $type = 'node' from menu_get_object().

apaderno’s picture

I still miss the point of having a content management system without the code to handle nodes (which are nothing else than content). How many people would use Drupal without nodes? It would not be possible to use CCK; Views would just be able to show a list of users, or taxonomy terms (which would not have any meaning, without nodes); every module that handles content depends on node.module.

The only case I can see, on using Drupal without node.module is for static output, or for output given by PHP files independent from Drupal; in both the cases, there would not be the need of using Drupal at all.

Xano’s picture

This patch is about making it optional, not about removing it.

apaderno’s picture

I understood that, but what is the purpose to make it optional when 99% of Drupal sites will need to enable it? It can only cause the modules to write more code just to verify node.module is active, or add a line more in their .info file.

Xano’s picture

Drupal doesn't need it, ergo it should not be required. That core currently depends on it is mostly due to a lot of cruft. Like the first post in this issue says, making it optional makes Drupal address a wider audience.

andypost’s picture

Good idea! Front page for anonymous is "mission", for authorized - /user so why /node not /user for anonymous?

Xano’s picture

FileSize
17.24 KB

This patch is an exact follow-up of the previous one. It applies and it works to a certain degree, but it has not been tested extensively.

Xano’s picture

Status: Needs work » Needs review
chx’s picture

A little research supports this patch. #21084: Node module is not marked "required", but is required by other required modules made node required because there were so many direct calls into the module however that clearly was before module dependencies got in. The oldest debate (2003) I was able to find is http://osdir.com/ml/php.drupal.devel/2003-12/msg00176.html and even there it was asked not to make it required.

Xano’s picture

FileSize
56.69 KB

New patch. I guess I'm about halfway in updating all code. All include files have been updated and all modules from Aggregator to Locale.

Some notes:
- node_types_rebuild() has gone from drupal_flush_all_caches(). This means we manually need to check of next to every call to drupal_flush_all_caches() if a call to node_types_rebuild() is desired.
- menu_rebuild() has gone from node_types_rebuild().
- menu_get_object($type = 'node', $position = 1, $path = NULL) is now menu_get_object($type, $position = 1, $path = NULL).
- I am unsure what to do with menu_tree_collect_node_links() and menu_tree_check_access(). I could wrap them in a lot of module_exist() calls, but that won't make things prettier.
- I am unsure what to do with the 'default_nodes_main' variable/setting in system_site_information_settings(). I don't see any existing page under Site configuration where it would fit.
- I am unsure what to do with 'toggle_node_user_pictures' in theme.inc due to lack of experience with themes. Is there a way to make themes provide this setting?
- {block_node_type} is now {node_block_visibility} and content type visibility settings are now set through hook_form_FORM_ID_alter() and enforced through hook_block_list_alter().
- Promoted to front page is now Promoted to /node. Although correct, I do feel we need to think of a better description.
- field_attach_prepare_translation() in field.attach.inc contains Node-specific code. There's a TODO comment that expresses concerns as to whether that particular function belongs in Field API or not.
- theme_options_none() accepts $node_type as third argument. The function is only being called from options.module. Can somebody confirm this $node_type argument can indeed be removed? There are other occurrences of node-specific code in options.module as well. I have no experience with Field whatsoever, so somebody else needs to check this out.
- The variable 'language_content_type_$type' has been renamed to 'node_type_language_$type', because its form elements have been moved from Locale to Node.
- Node has been renamed to Content. This is because with this patch Node appears in the modules list and without renaming it, it would have been the only occurence of 'node' in the UI. Bojhan and catch agree.

Status: Needs review » Needs work

The last submitted patch failed testing.

Xano’s picture

Issue tags: +makenodeoptional

The part of the patch that updates Locale has been moved to #540294: Move node language settings from Locale to Node module.

chx’s picture

Do not even think of touching promoted in this issue as it'll be a huge distraction. #404300: "Promote to front page" is a misnomer

Xano’s picture

The part of the patch that updates Locale has been moved to #543804: Decouple Block from Node.

Xano’s picture

Title: Make node module optional » Make Node.module optional

Path.module contains a lot of code that is specific for creating aliases for nodes. Since Path is an API and UI for creating aliases for any path, I believe it should not contain code to create paths for specific entities. How do you think about this?

The part of the patch that updates Help has been moved to #543906: Decouple Help from Node.

Xano’s picture

The part of the patch that updates Profile has been moved to #543920: Decouple Profile from Node.

Xano’s picture

The part of the patch that updates System has been moved to #545524: Decouple System from Node.

sun’s picture

Title: Make Node.module optional » Node module is not required (for Minimal install profile)
Category: feature » task
Status: Needs work » Needs review
FileSize
3.36 KB

Looking at the previous patch, this is probably missing a couple of bits. But still, HEAD runs just fine with this, and the goal should not be to remove it from the standard profile.

And here comes the reasoning: Today the idea was born to allow DrupalWebTestCases to specify

  $this->profile = 'minimal';
  parent::setUp();

because we have plenty of tests that just require a working database, but actually do not want to do anything with the standard profile. Hence, the smaller the minimal profile the better.

Additionally, this manifests the "entities are the new nodes" paradigm. Which is totally true.

Xano’s picture

Title: Node module is not required (for Minimal install profile) » Make Node.module optional (enabled for Standard profile)
Version: 7.x-dev » 8.x-dev

I changed the title a little so it reflects the purpose of this patch a bit more accurately. The goal is to make Node an optional module at all times, but we still want to have it installed when using the Standard profile for average Joe.

Also moving this to Drupal 8.

sun’s picture

Title: Make Node.module optional (enabled for Standard profile) » Node module is not required (for Minimal install profile)
Version: 8.x-dev » 7.x-dev
Assigned: Xano » sun
FileSize
14.64 KB

Not so fast...

And this even passed!

This means that plenty of tests are invalid. YAY! for invalid tests :)

Additionally, I can install Trigger module and access admin/structure/trigger/node, and guess what this shows me? Same for admin/structure/trigger/comment... totally a bug.

Also, WTF? After all non-node-dependent core modules + installing Search module + running cron + visiting status report I get plenty of requirement errors. "OpenID requires the BC Math library..." -- why was I able to install that module in the first place then?

Minor obstacles:

- Taxonomy terms can be assigned to users, but taxonomy/term/123 displays only nodes. WTF? Can be left to a contrib module though.

- Search module searches for nodes by default. No errors, just the default block form may need some tweaking.

...

As a result, I can totally install ALL core modules except those depending on Node: Blog, Book, Comment, Translation (fixed in contrib), Forum, Poll, Tracker.

On that note, not sure why Tracker depends on Comment (which depends on Node), but anyway...

This is just plain awesome!

webchick’s picture

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

Sorry, but we are well past the point in D7 for patches like this.

webchick’s picture

But it'd be nice to spin off some of those bug fixes you found into other issues so this is a quick commit in D8.

Xano’s picture

Title: Node module is not required (for Minimal install profile) » Make Node.module optional (enabled for Standard profile)

Xano: webchick: Hi there. Can we make Node.module optional for D7 or not?
webchick: Xano_, no.
*webchick kindly reminds everyone in the room that we have a fricking ALPHA coming out next week and would really appreciate help on actually critical bugs instead of nit-picking*

This is Drupal 8. We'd better let this rest until D7 comes out and we're free to mess up every API we want again.

- By the way: the original patch is so huge because a lot of code depends on Node being there at all times. There's stuff in the caching system, Block, Help, System, etc.

- Taxonomy's term pages work with nodes only. Hasn't got anything to do with this patch. Term pages just haven't been updated to use field API yet.

- I guess this might be the same reason for Search's misbehaviour.

sun’s picture

Version: 8.x-dev » 7.x-dev
FileSize
15.49 KB

This doesn't break anything serious - so why do we want to force users to install that bloated Node module?

Xano’s picture

There are a lot of things in my last patch that don't show up in yours. Are you sure you covered everything? Also, it might be best to decouple every module from Node in their own issues. I already created issues for every module, so we can reuse them. All issues related to making Node an optional module are tagged makenodeoptional.

webchick’s picture

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

* Because modules right now assume that node module always exists.
* Because the time for changes like this was before feature freeze.
* Because the time for changes that didn't make it in before feature freeze was by code slush.
* Because we already have over 50 issues that have introduced API changes in D7 that are not documented.
* Because there is absolutely no such thing as a patch that doesn't break anything. :P~

Do you need me to go on?

Seriously, can we please not fight about this? Can you not just respect my decision as the release manager? Can we not focus on much more pressing critical issues that are blocking D7's release?

webchick’s picture

I should point out, however, that despite my wish that people focus on critical bugs that are stressing me out ;) if Dries is really excited about this patch, he is of course free to overrule me, and there would be no hard feelings.

It's a great improvement, I just would feel better about it if some of the more pressing things on our plate were already dealt with.

sun’s picture

Version: 8.x-dev » 7.x-dev
FileSize
16.77 KB

Thanks, webchick! My point is: The testbot shows that this doesn't break anything. Additionally, I tested whether I can flawlessly work with Drupal without any Node-dependent modules - and it's perfectly possible. I can build a site just containing of blocks, aggregate stuff from other sites, have contact forms, create a community site with fieldable users... but most importantly: I am able to drive content on my site using some other entities than nodes... consider Panels, Views, etc.

I agree that it would be totally off to try to remove it from anything that core provides by default. However, what I'd really like to make happen is that experts are able to develop a custom install profile that doesn't use Node module.

Experts are surely able to work around the remaining - very minor - obstacles (such as replacing taxonomy/term/% output with non-node-based output).

- By the way: the original patch is so huge because a lot of code depends on Node being there at all times. There's stuff in the caching system, Block, Help, System, etc.

Most of those issues seem to be resolved already. As mentioned above, the overall system worked fine and I didn't see any errors or missing output or functionality anywhere.

- Taxonomy's term pages work with nodes only. Hasn't got anything to do with this patch. Term pages just haven't been updated to use field API yet.

Right. Just like I mentioned above, experts will be able to work around this, and we'll have to solve this in contrib anyway, since site builders most likely want to use the new taxonomy awesomeness (for users and any other entity) on their sites.

- I guess this might be the same reason for Search's misbehaviour.

That's already a critical issue due to other reasons: #567100: Search module is assuming node is default search even when it's disabled

So this patch really just allows Node to be not installed by default in custom installation profiles, which will be a huge benefit + paradigm shift for D7's release cycle.

This will allow us to think about and realize the overall entity + content situation much better and come up with some new ideas for D8 (or beyond).

catch’s picture

Status: Needs review » Needs work

I don't think at this stage, making all contrib modules which depend on node module being enabled (probably thousands) add it as a dependency is a great idea.

However, I don't see anything overly problematic in the patch apart from that - it doesn't have the menu_get_object() changes, rather than mess with the front page, it just returns a 404 (although that causes the site-information settings page to fail validation when you try to save it - if we actually had tests for that, they would fail, so CNW for that).

I think it'd be worth doing the cleanup here, leave node required, and let the super-advanced 0.01% of sites that don't want to use it hook_system_info_alter() it out of being required - then there's no API change, but it's technically possible to not use it without hacking core.

sun’s picture

Title: Make Node.module optional (enabled for Standard profile) » Allow Drupal core to run without Node module (don't make it optional)
Status: Needs work » Needs review
FileSize
14.75 KB

yeah, let's just do that. Still a major step into the right direction!

So all what this patch does now is to *allow* Drupal core to run without node.module without fatal errors.

In addition to the encountered bugs that were already stated in #53 (which need to be fixed elsewhere), this patch ensures that other modules can properly extend the block visibility settings with further vertical tabs.

Hence, a tiny step forward, but still very worth to do.

Jaza’s picture

Per catch's advice, I applied the latest patch (above), and created a new custom module called 'node_optional', which implements hook_system_info_alter(). Module code is as follows:

node_optional.info:

; $Id$
name = Node Optional
description = Makes the node module in Drupal core optional.
core = 7.x
files[] = node_optional.module

version = "7.x-1.x-dev"

node_optional.module:

// $Id$

/**
 * @file
 * Makes the node module in Drupal core optional.
 */

/**
 * Implements hook_system_info_alter().
 */
function node_optional_system_info_alter(&$info, $file, $type) {
  if ($type == 'module' && $file->name == 'node') {
    $info['required'] = FALSE;
  }
}

After creating and enabling this module, you can then see the node module on the module config page, and you can simply turn it off. I tried disabling the node module, and after doing so, I encountered no fatal errors (however I only tested a few pages).

While I'm in favour of this approach of keeping node module required (but allowing it to be made otherwise with a simple module such as node_optional), we should also remember that with the new fieldable entity system in D7, there may be more users than we expect who have no need for nodes, because they're using the Field API with various other (possibly custom) entities and not with nodes.

tstoeckler’s picture

If the approach stays that way (ie node module stays required) then the 4 line node_optional.module needs to be in contrib for other modules to depend on.

Xano’s picture

I am willing to 'maintain' a contrib Node Optional module. If more people agree this is the right approach, I'll commit the module to contrib.

Xano’s picture

I've been thinking a little. Whether we make Node truly optional or use contrib for that doesn't matter in terms of work we need to do for core. It's an extra one-line change in node.info. However, committing this change and making Node an optional module allows developers and site maintainers to easily enable and disable Node, as opposed to the contrib module solution, which I believe *will* be considered very nasty by a lot of developers. If we indeed do make it optional, maintainers will only have to add dependencies[] = node, something IMO hardly anyone will find hard or annoying to do.

sun’s picture

@Jaza: Exactly. That's why I want to get this in. There could even be a node2.module (just like there is a profile2.module) that implements "content" in a better way.

@Xano: We have a little chance to do this small step of allowing core to run without it. The current state of affairs is that you should know what you're doing (be an expert) or have a module like that node_optional.module installed to properly disable it and do not encounter false errors after doing so. There is no option to do more at this time.

So if we all agree, this is ready to fly?

Xano’s picture

What do you mean with false errors? This whole things works without errors, right? So we either go for dependencies or a hack in a contributed module. The former solution definitely sounds better to me.

Dave Reid’s picture

I think this is just too major of a decision and too rushed to land now. I would think something like this would need a lot of +1s from some more major players to get in now.

catch’s picture

@Xano, core works without errors, with this patch. Hundreds of contrib modules will be throwing fatal errors from whenever they do a node_load() until they declare a dependency, just to avoid a 10 line contrib module for the 0.01% of sites which might possibly run without node module enabled.

However, let's be clear, the patch in #63 doesn't make anything optional, it just makes core not break when you force node module to be disabled. I think that's a good thing, just a bit too late really - however I think it's up to webchick/Dries whether this should be moved up or not.

webchick’s picture

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

Ok, this is officially too late for Drupal 7 now. Only absolutely critical bug fixes are committed from here until tomorrow a.m. for alpha 1, and then we're focused solely on the critical queue.

But even in looking at the patch, it's won't fix for me as-is. It basically changes a bunch of node_foo()s to module_invoke('node', 'foo')s everywhere in core, which smells like an incredible hack to me. If we're going to really de-couple node from the core, and we're going to promote entities as the new way forward, then we need to do it right. That means major refactoring and de-coupling of the dependent subsystems, which can only happen in D8.

I also don't think this is terribly awful, tbh. It'll take awhile for the entity paradigm to set in, and D7 can act as a transition in the meantime while we figure all of that out.

sun’s picture

Extracted those very valuable changes to block node visibility settings into #684774: Block visibility settings cannot be properly extended

webchick’s picture

Title: Allow Drupal core to run without Node module (don't make it optional) » Make Node module optional

Fixing title, since I assume this is what we'll want in D8.

sun’s picture

Status: Needs work » Needs review
Issue tags: -makenodeoptional +Framework Initiative, +API clean-up
FileSize
4.85 KB

Time to revive this issue!

Status: Needs review » Needs work

The last submitted patch, drupal8.node-optional.75.patch, failed testing.

catch’s picture

Would be good to get this done.

Reviewing the patch:

Calling module_invoke() for things that aren't hooks, surely we can avoid at least some of these?

drupal_flush_all_caches() - looks like we need a hook_flush_all_caches_early() and hook_flush_all_caches_late(). Either that or move some of the hard coded stuff into system_flush_caches() so that we can mess with hook weighting (menu_rebuild() could probably go there). That is all a huge mess generally, if it's impossible to fix, then at least a big @todo above the line.

Reminds me I still haven't worked on hook_cache_bins() :( but that is not relevant.

Node access rebuild could surely happen in hook_modules_disabled()?

locale_uninstall() can't rely on node module being installed when it's uninstalled - node module could be disabled then module_invoke() would fail but you leave stale variables

That is part of #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed but it would be easy to write a locale test that passes with 8.x but fails with the patch applied.

sun’s picture

Status: Needs work » Needs review
FileSize
6.46 KB

Ran into #1093420: Recursive module dependencies of installation profile are not enabled in DrupalWebTestCase::setUp while attempting to fix the RDF test failure.

...and a module dependencies issue -- when installing completely unrelated modules, and one of them merely defines dependencies[], then that module is always installed last. Regardless of whether the other one has a larger weight. Just because it defines dependencies.

Debugging made me quickly invent #127641: Module weight in .info along the way. ;)

@catch's remarks are not contained yet.

sun’s picture

Let's see how this goes.

Status: Needs review » Needs work

The last submitted patch, drupal8.node-optional.79.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
7.65 KB

Wondering how many of those tests are using the Testing vs. Minimal profile. Let's see.

sun’s picture

Fixed {variables} should be {variable}.

sun’s picture

And as we already know from #913086: Allow modules to provide default configuration for running tests, we can safely fix drupalCreateUser() to make it work for the testing profile without Node module. :)

Status: Needs review » Needs work

The last submitted patch, drupal8.node-optional.83.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
9.99 KB

Alrighty, testing_install() is guilty, tries to grant anon and auth users the 'access content' permission by default - obviously breaking all tests depending on the testing profile. Attached patch should fix those.

In case it's going to pass, the only remaining @todo is the one about module_exists() in node_modules_disabled(). However, it might be possible that we can simply remove the condition and just check for module_hook(), since no actual functionality is invoked in there, and at the time of disabling a module, the hook implementation is still loaded in memory.

Status: Needs review » Needs work

The last submitted patch, drupal8.node-optional.85.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
12.78 KB

Fixed remaining failures in database test cases.

sun’s picture

Resolved and documented the remaining @todo as explained in #85.

catch’s picture

Patch looks good, however I'm sure there's some bits missing:

menu_get_object() - defaults to 'node'.

The default front page variable also defaults to 'node'.

RobLoach’s picture

Status: Needs review » Needs work
+++ b/includes/common.incundefined
+++ b/includes/common.incundefined
@@ -7129,7 +7129,11 @@ function drupal_flush_all_caches() {

@@ -7129,7 +7129,11 @@ function drupal_flush_all_caches() {
   system_rebuild_theme_data();
   drupal_theme_rebuild();
 
-  node_types_rebuild();
+  // @todo D8: Split cache flushing from rebuilding.
+  // @see http://drupal.org/node/996236
+  if (module_exists('node')) {
+    node_types_rebuild();

Could this be moved to a new node_flush_caches() function? hook_flush_caches() is called in the drupal_flush_all_caches() function.

19 days to next Drupal core point release.

sun’s picture

Status: Needs work » Needs review

@catch: Not sure whether this needs to be part of this issue? Regarding menu_get_object(), I think we have another issue in the queue already (or Entity module's?) to abstract that into a entity_get_object() or whatever, and lastly, it's going to be changed and invalidated by contexts anyway...

Regarding the front page, ugh. -- Change the default to 'user', and make Minimal + Standard profiles set it to 'node'...?

@Rob Loach: The intention of the explicitly added @todo and @see to #996236: drupal_flush_all_caches() does not clear entity info cache was to clarify that this is a separate, huge undertaking. Additionally, the comment following right afterwards, visible in the diff context, clarifies even more that node_types_rebuild() cannot be simply shuffled around:

+++ b/includes/common.inc
@@ -7129,7 +7129,11 @@ function drupal_flush_all_caches() {
-  node_types_rebuild();
+  // @todo D8: Split cache flushing from rebuilding.
+  // @see http://drupal.org/node/996236
+  if (module_exists('node')) {
+    node_types_rebuild();
+  }
   // node_menu() defines menu items based on node types so it needs to come
   // after node types are rebuilt.
   menu_rebuild();

drupal_flush_all_caches() is a big pile of mostly unknown interdependencies right now. Fixing that is way out of scope for this issue.

mcrittenden’s picture

Sub.

sun’s picture

Attached patch additionally changes the default site_frontpage from 'node' to 'user'. Standard and Minimal installation profiles set it to 'node', retaining current behavior.

This has a UX implication on the Site information settings form: In D7, we're "hiding" the default path 'node' and present an empty form value instead. If the value remains empty, the variable is set back to 'node'. This behaves identically now, but for 'user' instead of 'node'.

catch’s picture

If the default front page variable is empty string is that a 404? It might be OK to require profile authors to set this...

The menu get object isn't a dependency so there is not much harm leaving it in. Not at pc so haven't reviewed latest patch.

sun’s picture

Also had to double-check what the menu router's behavior on an empty path is. And correct, the answer is 404. An empty $_GET['q'] cannot be mapped to a router item. That's something to look into at some point in the future; my personal expectation for an empty path was something along the lines of an empty page (as if returning NULL from a page callback).

catch’s picture

Just came across #1187906: Shortcut module cannot be installed via an install profile if the menu module wasn't installed first - that issue moves the default node/add link to standard.install which is probably the right fix for this issue as well.

catch’s picture

sun’s picture

The change from #1177830: shortcut_install() should be in standard_install() should be done separately in that issue IMHO. Whichever patch lands first has to be re-rolled, as usual.

sun’s picture

Re-rolled against latest branch HEAD.

sun’s picture

Re-rolled against latest branch HEAD.

RobLoach’s picture

There was a link to node/add on the Help page even though Node was disabled. Looks pretty good to me. What else is missing?

anavarre’s picture

Subscribe

dasjo’s picture

sun’s picture

Status: Needs review » Needs work
Issue tags: +Framework Initiative, +API clean-up

The last submitted patch, drupal8.node-optional.101.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
20.23 KB

Status: Needs review » Needs work

The last submitted patch, drupal8.node-optional.106.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
22.13 KB

Fixed those wonky tests.

sun’s picture

Final reviews, anyone?

Xano’s picture

Status: Needs review » Needs work

This is just a short review of the patch code. I haven't applied or tested the patch locally.

+++ b/core/modules/help/help.module
@@ -43,7 +43,10 @@ function help_help($path, $arg) {
+      // Display a link to the create content page if Node module is enabled.
+      if (module_exists('node')) {
+        $output .= '<li>' . t('<strong>Start posting content</strong> Finally, you can <a href="@content">add new content</a> for your website.', array('@content' => url('node/add'))) . '</li>';
+      }

This *should* not be here. Can we move this to node_help instead?

+++ b/core/modules/shortcut/shortcut.install
@@ -13,18 +13,19 @@ function shortcut_install() {
-  $shortcut_set->links = array(
-    array(
+  $shortcut_set->links = array();
+  if (module_exists('node')) {
+    $shortcut_set->links[] = array(
       'link_path' => 'node/add',
       'link_title' => $t('Add content'),
       'weight' => -20,
-    ),
-    array(
+    );
+    $shortcut_set->links[] = array(
       'link_path' => 'admin/content',
       'link_title' => $t('Find content'),
       'weight' => -19,
-    ),
-  );
+    );
+  }

I would move this to a dedicated function in node.module and make node_install() and node_modules_enabled() call it.

+++ b/profiles/minimal/minimal.info
@@ -2,6 +2,7 @@ name = Minimal
+dependencies[] = node

This is subject to debate, but IMHO Node does not belong in the minimal profile.

+++ b/profiles/minimal/minimal.install
@@ -66,6 +66,9 @@ function minimal_install() {
+  // Set front page to "node".
+  variable_set('site_frontpage', 'node');
+

Which means these lines should be removed from minimal.install as well.

16 days to next Drupal core point release.

sun’s picture

Status: Needs work » Needs review

Can we move this to node_help instead?

Unfortunately not. This requires a larger API change to hook_help(), since modules are currently returning strings. This particular help text fragment is a bullet point in a HTML list.

Thus, this node-specific help text can only be moved to node_help(), if modules are able to return renderable arrays in hook_help(), and, when that renderable array is passed to a hook_help_alter() afterwards.

We can and should discuss and work on that in a follow-up issue. Until then, the world won't end because of that condition in help_help().

I would move this to a dedicated function in node.module and make node_install() and node_modules_enabled() call it.

Won't be part of this issue nor patch; see #1177830: shortcut_install() should be in standard_install()

This is subject to debate, but IMHO Node does not belong in the minimal profile.

That's a debate for a separate issue; doesn't matter for this issue.

Tor Arne Thune’s picture

+++ b/core/modules/rdf/tests/rdf_test.info
@@ -4,3 +4,8 @@ package = Testing
+; module_enable() sorts modules with dependencies _always_ after modules without
+; dependencies. Blog module depends on Node module, so it is sorted last.
+; rdf_test module attempts to override Blog module's RDF mapping, so a
+; dependency is needed to sort it into "groups of modules having dependencies".

Blog module is no longer a part of Drupal 8.

Other than that, nothing caught my eye.

moshe weitzman’s picture

Status: Needs review » Needs work

I read through this and it looks like a nice cleanup. I found nothing objectionable at all. We should edit or remove code based on #112 and then this is RTBC.

Xano’s picture

Status: Needs work » Needs review
Issue tags: -Framework Initiative, -API clean-up

#108: drupal8.node-optional.108.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Framework Initiative, +API clean-up

The last submitted patch, drupal8.node-optional.108.patch, failed testing.

Xano’s picture

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

Rerolled the patch since it didn't apply. I'll deal with the Blog module reference in another issue. It's no part of this one.

Xano’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
20.06 KB

Scratch that. I hadn't seen the Blog comments were part of the patch. They're removed in this one.

sun’s picture

+++ b/core/modules/rdf/tests/rdf_test.info
@@ -4,3 +4,4 @@ package = Testing
+dependencies[] = rdf

Did you manually test whether the addition of this dependency is actually still required now?

That said, it surely can't hurt to properly define dependencies for testing modules, so I'm perfectly fine to leave that in.

What I'm after is that I'd like to know whether that RDF mapping test was properly adjusted during removal of Blog module.

Xano’s picture

There are no references to Blog module left in rdf_test module. The dependency in its info file should not matter, since the tests' setUp() methods explicitely install RDF module, but it can't hurt to leave it in, if only for documentation purposes.

Xano’s picture

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Good to go.

andypost’s picture

In change notification should be pointed that front page is set to 'node' ONLY if standard profile is used.

+++ b/profiles/standard/standard.installundefined
@@ -199,6 +199,9 @@ function standard_install() {
+  // Set front page to "node".
+  variable_set('site_frontpage', 'node');
+

Maybe this should live in node.install?

moshe weitzman’s picture

IMO it is up to the profile to declare its frontpage so the patch is correct. It is node's job just to offer the page at /node

catch’s picture

Status: Reviewed & tested by the community » Needs work

No longer applies (locale.install).

Dave Reid’s picture

Title: Make Node module optional » [ROLLBACK] Make Node module optional
Priority: Normal » Critical
webchick’s picture

Title: [ROLLBACK] Make Node module optional » Make Node module optional
Priority: Critical » Normal

I reverted this, since it looks like it was accidentally committed.

git checkout 8.x; git pull origin; git revert 9db1fddb9067; git commit; git push origin 8.x

(Thanks, sun!)

sun’s picture

Status: Needs work » Needs review
Issue tags: +Testing system
FileSize
20.87 KB

Re-rolled against HEAD, and fixed a range of test failures caused by recent commits; FYI:

1) Any variable_get('site_frontpage', 'node') needs to be variable_get('site_frontpage', 'user').

2) Most tests that use the Testing profile needs to additionally install 'node'. This is a known issue. See the action plan over in #1373142: Use the Testing profile. Speed up testbot by 50%

catch’s picture

Whoops sorry folks, I was thinking about committing this but didn't mean to last night, looks like this was all fixed overnight.

sun’s picture

Status: Needs review » Reviewed & tested by the community

This was RTBC before, and only had to be re-rolled for conflicting changes in the diff context.

The test failures reported in a separate issue are obsolete. Another recent commit changed FieldUITestCase to use the Testing profile, so it had to be adjusted to additionally install Node module.

xjm’s picture

So the functional changes introduced by this patch are:

  1. node.module becomes optional.
    • Profiles which use node need to specify it as a dependency. The minimal and standard profiles already include this change.
    • Contrib modules that expect nodes to exist need to specify node as a dependency or execute node-related code conditionally.
    • Tests that use the testing profile must enable node (doesn't that sound weird?) in order to use its functionality. For the interim, the testing profile can specify node as a dependency.
  2. The default front page is the user page.
    • Profiles which use node as a front page need to define the site_frontpage config variable appropriately. The minimal and standard profiles already include this change.

I've a couple questions yet about enabling and disabling the node module... e.g., do we need to add automatically granting 'access content' permission in a hook_enable() hook_install() implementation? And what actions might we need to take when disabling or uninstalling the module? I don't see any of those hook implementations in the patch.

xjm’s picture

Issue tags: +Needs change record

And, yeah. Tagging. :P

catch’s picture

Status: Reviewed & tested by the community » Needs work

That's a good point, we should be cleaning up variables in hook_module_uninstalled() at least.

sun’s picture

Status: Needs work » Needs review
FileSize
22.19 KB

Attached patch adds a node_uninstall() implementation to delete all variables.

Regarding hook_install() and permissions: Modules should never attempt to auto-grant permissions to any role on installation. The configuration of user roles and permissions is the responsibility of the installation profile, or the site builder/administrator when installing a(ny) module at a later point in time.

xjm’s picture

Well, in that case, shouldn't we be baking access content into Standard and Minimal? Did I miss that in the patch?

Edit: That's already there in core, of course, or nothing would work. Please disregard. :)

sun’s picture

webchick’s picture

Really not trying to be a 'negative nelly' here, but how can a module_exists('node') check in common.inc mean that this module is truly optional? Should this not be postponed on #996236: drupal_flush_all_caches() does not clear entity info cache?

It also feels like we need an upgrade path here for sites that have never set the site_frontpage variable. Else, suddenly upon upgrade their home page is going to flip to /user out from under them, which for anonymous users is going to be a login form, not their home page with their site's actual content.

webchick’s picture

Status: Needs review » Needs work

I guess that's...

sun’s picture

Status: Needs work » Needs review
FileSize
23 KB

Added node_update_8000() to explicitly configure the site_frontpage if it implicitly was 'node' before.

And yes, aside from (most) tests, there's a lot more to untangle and clean up further afterwards. Sadly, Node module is very similar to System module currently; it's all over the place. -- However, we need to start somewhere.

sun’s picture

Any other feedback?

Anonymous’s picture

this looks good to me, the only thing i was wondering - what about node_admin_theme?

do we need something like this in system_themes_admin_form():

  if (module_exists('node')) {
    $form['admin_theme']['node_admin_theme'] = array(
      '#type' => 'checkbox',
      '#title' => t('Use the administration theme when editing or creating content'),
      '#default_value' => variable_get('node_admin_theme', '0'),
    );
  }
RobLoach’s picture

FileSize
25.09 KB

this looks good to me, the only thing i was wondering - what about node_admin_theme?

Should probably move that out of System module entirely and into Node module itself. This patch includes a node_form_system_themes_admin_form_alter() and node_form_system_themes_admin_form_submit().

Status: Needs review » Needs work

The last submitted patch, 375397.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
26.07 KB

Node module not enabled during the administration theme tests?

Status: Needs review » Needs work

The last submitted patch, 375397.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
25.05 KB
+++ b/core/modules/system/system.admin.inc
@@ -277,9 +272,9 @@ function system_themes_admin_form($form, &$form_state, $theme_options) {
 function system_themes_admin_form_submit($form, &$form_state) {
+dsm($form);

Dude!

Attached patch should pass again, based on #141.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

ok, this looks good to me, #140 was my only issue, RTBC.

RobLoach’s picture

Haha, whoops :-) . Good catch!

catch’s picture

Title: Make Node module optional » Change notice for: Make Node module optional
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: -Needs change record

待てました!

Committed/pushed to 8.x, thanks!

This needs a change notification so tell modules to add a dependency. Also seems worth a changelog entry?

xjm’s picture

Issue tags: +Needs change record

Yay!

(Putting the tag back.)

sun’s picture

Title: Change notice for: Make Node module optional » Make Node module optional
Status: Active » Fixed
Issue tags: -Needs change record
sun’s picture

Status: Fixed » Closed (fixed)
Issue tags: -Framework Initiative, -API clean-up, -Testing system

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

xjm’s picture

Component: node.module » node system
Issue summary: View changes

xjm’s picture