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.
Comment | File | Size | Author |
---|---|---|---|
#145 | drupal8.node-optional.145.patch | 25.05 KB | sun |
#143 | 375397.patch | 26.07 KB | RobLoach |
#141 | 375397.patch | 25.09 KB | RobLoach |
#138 | drupal8.node-optional.138.patch | 23 KB | sun |
#133 | drupal8.node-optional.133.patch | 22.19 KB | sun |
Comments
Comment #1
XanoThis 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.
Comment #3
catchEven without decoupling taxonomy from node, it could always have node as a dependency until that's done.
Comment #4
XanoWhat 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.
Comment #5
XanoSlightly 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.
Comment #7
XanoSimplified 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.
Comment #8
XanoAll 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.
Comment #9
catchI'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.
Comment #10
XanoIMO the Expert profile should only enable the absolutely necessary modules and Block, because the interface is nothing without a module that displays menus.
Comment #11
XanoDiscussed this with catch and we agreed that change belongs in a different issue. This patch updates the Expert install profile.
Comment #12
tstoecklerWhen 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?
Comment #13
XanoThe 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.
Comment #14
tstoecklerSorry...
Comment #15
XanoMoved all the (un)installation code from system.install to node.install. Functionality seems to work. Let's see what our bot says.
Comment #17
XanoDefining 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.
Comment #18
XanoInstall 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
Comment #20
XanoMy 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.
Comment #21
XanoThe 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 )
Comment #23
XanoAdding an omitted drupal_set_title() prevents a lot of errors.
Comment #25
XanoThe frontpage issue is moved to #419950: Remove /node as default front page.
Comment #26
tstoecklerStill 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...
Comment #27
XanoGood observation! They shouldn't be available at all after uninstallation.
Comment #28
Dave ReidThere's a patch for that: #420124: DX: Remove module actions on uninstall
Comment #30
XanoComment #32
Jaza CreditAttribution: Jaza commentedThis 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 tohook_uninstall()
in node module, telling users that they might need to re-configure their front page setting.Comment #33
XanoThe 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.
Comment #34
XanoNote: remove
$type = 'node'
from menu_get_object().Comment #35
apadernoI 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.
Comment #36
XanoThis patch is about making it optional, not about removing it.
Comment #37
apadernoI 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.
Comment #38
XanoDrupal 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.
Comment #39
andypostGood idea! Front page for anonymous is "mission", for authorized - /user so why /node not /user for anonymous?
Comment #40
XanoThis 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.
Comment #41
XanoComment #42
chx CreditAttribution: chx commentedA 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.
Comment #43
XanoNew 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.
Comment #45
XanoThe part of the patch that updates Locale has been moved to #540294: Move node language settings from Locale to Node module.
Comment #46
chx CreditAttribution: chx commentedDo not even think of touching promoted in this issue as it'll be a huge distraction. #404300: "Promote to front page" is a misnomer
Comment #47
XanoThe part of the patch that updates Locale has been moved to #543804: Decouple Block from Node.
Comment #48
XanoPath.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.
Comment #49
XanoThe part of the patch that updates Profile has been moved to #543920: Decouple Profile from Node.
Comment #50
XanoThe part of the patch that updates System has been moved to #545524: Decouple System from Node.
Comment #51
sunLooking 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
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.
Comment #52
XanoI 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.
Comment #53
sunNot 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!
Comment #54
webchickSorry, but we are well past the point in D7 for patches like this.
Comment #55
webchickBut 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.
Comment #56
XanoThis 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.
Comment #57
sunThis doesn't break anything serious - so why do we want to force users to install that bloated Node module?
Comment #58
XanoThere 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.
Comment #59
webchick* 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?
Comment #60
webchickI 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.
Comment #61
sunThanks, 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).
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.
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.
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).
Comment #62
catchI 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.
Comment #63
sunyeah, 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.
Comment #64
Jaza CreditAttribution: Jaza commentedPer 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:
node_optional.module:
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.
Comment #65
tstoecklerIf 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.
Comment #66
XanoI 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.
Comment #67
XanoI'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.Comment #68
sun@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?
Comment #69
XanoWhat 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.
Comment #70
Dave ReidI 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.
Comment #71
catch@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.
Comment #72
webchickOk, 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.
Comment #73
sunExtracted those very valuable changes to block node visibility settings into #684774: Block visibility settings cannot be properly extended
Comment #74
webchickFixing title, since I assume this is what we'll want in D8.
Comment #75
sunTime to revive this issue!
Comment #77
catchWould 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.
Comment #78
sunRan 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.
Comment #79
sunLet's see how this goes.
Comment #81
sunWondering how many of those tests are using the Testing vs. Minimal profile. Let's see.
Comment #82
sunFixed {variables} should be {variable}.
Comment #83
sunAnd 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. :)
Comment #85
sunAlrighty, 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.
Comment #87
sunFixed remaining failures in database test cases.
Comment #88
sunResolved and documented the remaining @todo as explained in #85.
Comment #89
catchPatch 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'.
Comment #90
RobLoachCould 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.
Comment #91
sun@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:
drupal_flush_all_caches() is a big pile of mostly unknown interdependencies right now. Fixing that is way out of scope for this issue.
Comment #92
mcrittenden CreditAttribution: mcrittenden commentedSub.
Comment #93
sunAttached 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'.
Comment #94
catchIf 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.
Comment #95
sunAlso 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).
Comment #96
catchJust 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.
Comment #97
catchWrong issue, I meant #1177830: shortcut_install() should be in standard_install().
Comment #98
sunThe 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.
Comment #99
sunRe-rolled against latest branch HEAD.
Comment #100
sunRe-rolled against latest branch HEAD.
Comment #101
RobLoachThere was a link to node/add on the Help page even though Node was disabled. Looks pretty good to me. What else is missing?
Comment #102
anavarreSubscribe
Comment #103
dasjoComment #104
sun#101: drupal8.node-optional.101.patch queued for re-testing.
Comment #106
sunComment #108
sunFixed those wonky tests.
Comment #109
sunFinal reviews, anyone?
Comment #110
XanoThis is just a short review of the patch code. I haven't applied or tested the patch locally.
This *should* not be here. Can we move this to node_help instead?
I would move this to a dedicated function in node.module and make node_install() and node_modules_enabled() call it.
This is subject to debate, but IMHO Node does not belong in the minimal profile.
Which means these lines should be removed from minimal.install as well.
16 days to next Drupal core point release.
Comment #111
sunUnfortunately 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().
Won't be part of this issue nor patch; see #1177830: shortcut_install() should be in standard_install()
That's a debate for a separate issue; doesn't matter for this issue.
Comment #112
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedBlog module is no longer a part of Drupal 8.
Other than that, nothing caught my eye.
Comment #113
moshe weitzman CreditAttribution: moshe weitzman commentedI 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.
Comment #114
Xano#108: drupal8.node-optional.108.patch queued for re-testing.
Comment #116
XanoRerolled 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.
Comment #117
XanoScratch that. I hadn't seen the Blog comments were part of the patch. They're removed in this one.
Comment #118
sunDid 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.
Comment #119
XanoThere 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.
Comment #120
XanoAlso see #1370708: Remove Node module from Minimal installation profile (re: #11)
Comment #121
moshe weitzman CreditAttribution: moshe weitzman commentedGood to go.
Comment #122
andypostIn change notification should be pointed that front page is set to 'node' ONLY if standard profile is used.
Maybe this should live in node.install?
Comment #123
moshe weitzman CreditAttribution: moshe weitzman commentedIMO 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
Comment #124
catchNo longer applies (locale.install).
Comment #125
Dave ReidThis was accidentally applied along with #1368872: Clean up API docs for syslog
http://drupalcode.org/project/drupal.git/commit/9db1fddb906738e6c8873cba...
Also this would indicate we're getting random test failures with this patch applied in core: #1380752: Possible bad tests or core bug: FieldUIManageFieldsTestCase and ThemeFunctionsTestCase failing sporadically on multiple testbots
Comment #126
webchickI reverted this, since it looks like it was accidentally committed.
(Thanks, sun!)
Comment #127
sunRe-rolled against HEAD, and fixed a range of test failures caused by recent commits; FYI:
1) Any
variable_get('site_frontpage', 'node')
needs to bevariable_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%
Comment #128
catchWhoops sorry folks, I was thinking about committing this but didn't mean to last night, looks like this was all fixed overnight.
Comment #129
sunThis 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.
Comment #130
xjmSo the functional changes introduced by this patch are:
node.module
becomes optional.node
need to specify it as a dependency. The minimal and standard profiles already include this change.node
as a front page need to define thesite_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 ahook_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.Comment #131
xjmAnd, yeah. Tagging. :P
Comment #132
catchThat's a good point, we should be cleaning up variables in hook_module_uninstalled() at least.
Comment #133
sunAttached 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.
Comment #134
xjmWell, in that case, shouldn't we be bakingaccess 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. :)
Comment #135
sun@xjm: Yep, that already exists:
http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/profiles/s...
http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/profiles/m...
Comment #136
webchickReally 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.
Comment #137
webchickI guess that's...
Comment #138
sunAdded 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.
Comment #139
sunAny other feedback?
Comment #140
Anonymous (not verified) CreditAttribution: Anonymous commentedthis 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():
Comment #141
RobLoachShould probably move that out of System module entirely and into Node module itself. This patch includes a
node_form_system_themes_admin_form_alter()
andnode_form_system_themes_admin_form_submit()
.Comment #143
RobLoachNode module not enabled during the administration theme tests?
Comment #145
sunDude!
Attached patch should pass again, based on #141.
Comment #146
Anonymous (not verified) CreditAttribution: Anonymous commentedok, this looks good to me, #140 was my only issue, RTBC.
Comment #147
RobLoachHaha, whoops :-) . Good catch!
Comment #148
catch待てました!
Committed/pushed to 8.x, thanks!
This needs a change notification so tell modules to add a dependency. Also seems worth a changelog entry?
Comment #149
xjmYay!
(Putting the tag back.)
Comment #150
sunTwo change notices:
http://drupal.org/node/1397856
http://drupal.org/node/1397862
Comment #151
sunSorry, critical follow-up: #1397954: Temporarily add Node module to Testing profile
Comment #153
xjmComment #155
xjm