As per title. We should define page and story programmatically in default.profile rather than as DB inserts in system.install.

Can we do this like in system update 1005? How much should be hardcoded in system.install vs. in default.profile? Ideally, I'm thinking we should use our API in default.profile. Steven had concerns about db inserts and having to maintain queries, but I'm looking to optimize for "cut and paste" install profile creators...so we should be "eating our own dogfood" in default.

I don't think I can call this a bug....but it certainly won't help spur development of simple install profiles.

Aside: do we need a new component for "install system"?

  $types = array(
    array(
      'type' => 'page',
      'name' => t('Page'),
      'module' => 'node',
      'description' => t('If you want to add a static page, like a contact page or an about page, use a page.'),
      'custom' => TRUE,
      'modified' => TRUE,
      'locked' => FALSE,
    ),
    array(
      'type' => 'story',
      'name' => t('Story'),
      'module' => 'node',
      'description' => t('Stories are articles in their simplest form: they have a title, a teaser and a body, but can be extended by other modules. The teaser is part of the body too. Stories may be used as a personal blog or for news articles.'),
      'custom' => TRUE,
      'modified' => TRUE,
      'locked' => FALSE,
    )
  );

  foreach ($types as $type) {
    $type = (object) _node_type_set_defaults($type);
    node_type_save($type);
  }

  cache_clear_all();
  system_modules();
  menu_rebuild();
  node_types_rebuild();
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Boris Mann’s picture

Various attempts to do this haven't worked yet. Commenting out lines 1091 and 1092 which do the inserts --

  db_query("INSERT INTO {node_type} (type, name, module, description, help, has_title, title_label, has_body, body_label, min_word_count, custom, modified, locked, orig_type) VALUES ('page', 'Page', 'node', 'If you want to add a static page, like a contact page or an about page, use a page.', '', 1, 'Title', 1, 'Body', 0, 1, 1, 0, 'page')");
  db_query("INSERT INTO {node_type} (type, name, module, description, help, has_title, title_label, has_body, body_label, min_word_count, custom, modified, locked, orig_type) VALUES ('story', 'Story', 'node', 'Stories are articles in their simplest form: they have a title, a teaser and a body, but can be extended by other modules. The teaser is part of the body too. Stories may be used as a personal blog or for news articles.', '', 1, 'Title', 1, 'Body', 0, 1, 1, 0, 'story')");

-- correctly makes it so that no content types are enabled.

Both db_query and _node_type_set_defaults($type) plus node_type_save($type) don't work in default_profile_install(), both with and without a call to drupal_bootstrap().

Stuck until we can figure out what's going on. Ideally, the programmatic way of setting node defaults should be used, as listed in the initial issue. We can make a function called _default_create_node_types() and call that from system_update_1005() to keep the code for creation in one place.

pwolanin’s picture

I agree that we should move toward only creating types programatically- ideally this should happen at the very end of the install, so that various modules could responding to the creation of the node type via hook_node_type.

RobRoy’s picture

Version: 5.0-rc1 » 5.x-dev
Category: feature » task
Status: Active » Needs review
FileSize
3.01 KB

I just moved the db inserts from system_install to default_install as this was the easiest. We could use the API in default_profile_final also, but I think we'd have to do an include_once './'. drupal_get_path('module', 'node') .'/content_types.inc'; so this just seemed the cleanest approach for now.

RobRoy’s picture

Status: Needs review » Needs work

@pwolanin - I didn't see you post before I submitted that patch. That's a good point. I'll roll another one to do this with the API in a bit.

RobRoy’s picture

Status: Needs work » Needs review
FileSize
4.01 KB

New patch using API functions and the default_profile_final() which offers a full bootstrap. Also, I'm pretty sure we can use t() instead of st() in this function safely because of this. Looks like we could also use t() in install_complete() for the same reason. Should I be using st() here just to be consistent, or is the rule if you can use t() go for it?

Boris Mann’s picture

Ran through this patch, verified that it works as expected. Thanks for figuring this out, RobRoy....I was stuck. Now to look at making those page changes.

RobRoy’s picture

I figure that belongs in a separate issue. Do you have one opened for that yet Boris?

Dries’s picture

Version: 5.x-dev » 6.x-dev

Moving to D6.

Boris Mann’s picture

Version: 6.x-dev » 5.x-dev

@RobRoy: don't have a separate issue for changing workflow yet. I think it's directly related to the "definition" of page and story, so, shouldn't it be in this one?

@Dries: no way we can see this in 5? It means we go for a whole cycle not using best practices for in our default profile. Setting back to 5.x to see if we can get further comment.

drumm’s picture

Version: 5.x-dev » 6.x-dev

Moving this line

return '<p>'. (drupal_set_message() ? t('Please review the messages above before continuing on to <a href="@url">your new site</a>.', array('@url' => url(''))) : t('You may now visit <a href="@url">your new site</a>.', array('@url' => url('')))) .'</p>';

is an API change.

RobRoy’s picture

@drumm - If I refactored install_complete() to check if profilename_profile_final() returns anything, would that work? I think that's a better way to go anyways. Something like this?

  if (function_exists($function)) {
    // More steps required
    $profile_message = $function();
  }
  if (isset($profile_message)) {
    $output .= $profile_message;
  }
  else {
    // No more steps
    $output .= '<p>' . (drupal_set_message() ? st('Please review the messages above before continuing on to <a href="@url">your new site</a>.', array('@url' => url(''))) : st('You may now visit <a href="@url">your new site</a>.', array('@url' => url('')))) . '</p>';
  }
RobRoy’s picture

Version: 6.x-dev » 5.x-dev
FileSize
4.01 KB

Fix to not change API (hopefully). Moving back to 5.x to see if this is valid again.

RobRoy’s picture

Oh, and there is a related patch at http://drupal.org/node/105365 which sets page defaults to non-promoted and comments disabled.

RobRoy’s picture

FileSize
4.32 KB

Okay, here is the merged patch with 'page' defaults. I was doing this but it sets a message about Page type being updated so I just set the variables directly since that's only 2 lines. :P

include_once './'. drupal_get_path('module', 'node') .'/content_types.inc';

  $type = node_get_types('type', 'page');
  $form_values = array(
    'node_options' => array('status' => 1, 'promote' => 0),
    'comment' => COMMENT_NODE_DISABLED,
  );
  drupal_execute('node_type_form', $form_values, $type);
Dries’s picture

Boris: I'm willing to make an exception if Neil is willing too. I think that getting the workflow right is important because it confuses a lot of people, and probably not a world-breaking API change.

Boris Mann’s picture

Great to hear, Dries -- doesn't look like an API change at all, just moving code to default.profile out of system.install, plus of course agreeing on the workflow options.

I'll need to sit down and test this.

RobRoy’s picture

I fixed the API change on the install_complete() return value drumm was talking about so I don't even think we have to worry on that end. It would be cool to have default.profile a working installation profile example like this.

Anonymous’s picture

/

Anonymous’s picture

@Boris: I figured out what irked me most with the new Page/Story methods. See http://drupal.org/node/106396 for follow-up.

Anonymous’s picture

@Boris: You stated in the email that Page should have Revisions on by default. RobRoy's profile_33.patch doesn't do that.

RobRoy’s picture

This patch and issue just focuses on moving the current definitions of page and story to default.profile. If you have another issue regarding revisions, please, please, please open another issue. :) We try to keep patches as small and focused as practical and revisions should definitely be another issue. I'd like to get some other users to test this patch on new installations and confirm that it works. Then, we can set this to RTBC and move on to other issues. Thanks.

Anonymous’s picture

Can we change

+      'description' => t('Stories are articles in their simplest form: they have a title, a teaser and a body, but can be extended by other modules. The teaser is part of the body too. Stories may be used as a personal blog or for news articles.'),

to

+      'description' => t('Stories are articles that are promoted to the front page: they have a title, a teaser and a body, but can be extended by other modules. The teaser is part of the body too and is controlled by the Post Settings menu "Length of trimmed posts" field or by <break> in the story body if you need to force a longer teaser due to HTML formatting. Stories may be used as a personal blog or for news articles.'),
Anonymous’s picture

@RobRoy: Boris asked me to comment here. Your patch doesn't fit our (Boris and my) conversation and I wanted clarification. Also based on #15 the issue has become broader to include fixing the workflow issues of which Revisions is about.

Anonymous’s picture

BTW, I did test the patch on a new installation and that is why I am commenting. The patch worked except that Boris had stated the Page node type would default to Revisions which your patch doesn't do.

webchick’s picture

earnie: No, there can be no changes to strings, unless it's a blatant bug. We're in a string freeze. Please see http://drupal.org/node/89196 where effort is being directed toward cleaning all of these up for 6.x.

I tend to also lean against implementing revisions out-of-the-box on page nodes. Most users don't need this level of control, and the extra tab will be confusing to them. Those that do can tick a box.

HUGE +1 for this patch. The very first thing I do every time I install Drupal is set these workflow options so that page nodes actually do what the description says they should. In addition, we get a nice example right in core on how to set post-install options for an install profile (for instance, I thought it was profile_name_install(), not profile_name_final()). And as an extra bonus, an example on submitting a form programmatically. Awesome!

Patch still applies with offset. Pages get posted with comments disabled and not promoted to front page, as advertised.

Two items of feedback:

a) I would also turn off "Display post information on" for page nodes on the global theme settings. While this may just be personal preference, this does come up in the forums A LOT. It shouldn't hold the patch though if there's going to be bickering on it.
b) Because pages are not promoted to the front page, there are two consequences:
1. Pages get "lost" -- if you didn't make a menu entry for a page, once you click away from a page, there is no way to get to it again apart from manually typing in "node/1" and/or going to admin/content/node. This is expected behaviour for people like you and I, but may be really confusing for new users.
2. The welcome message reads: "This message will disappear once you have published your first post," which is not true. We knew this wasn't true before as well, but before everything was promoted to front page by default. Now that there's a diversion, we might need to look at rephrasing that somehow (though "once you make your first post which is promoted to front page" is icky...)

I don't think the items at b) should hold up this patch necessarily, but we probably would need to address them in subsequent patches before 5.0 release.

Leaving "needs review" rather than "RTBC" just to get some feedback on these points from some other folks.

RobRoy’s picture

FileSize
4.55 KB

@earnie: Fair enough on the Revisions thing. But I do agree with webchick that we shoudn't do revisions out-of-the-box so in my mind that requires more feedback and I think would be better suited in another issue so it doesn't hold this one up.

@webchick: Excellent points, this patch sets Page post information to FALSE. Will think about other points a bit later.

Anonymous’s picture

"This message will disappear once you have published your first post,"

How about "This message will disappear once you have published your first story," instead?

RobRoy’s picture

I think we should go towards the "icky" direction since admins can change default options and should say...

"This message will disappear once you have promoted a post to the front page."

I like it, but think it should go in another issue (see http://drupal.org/node/106428). (Don't I sound like a broken record?) :P

Boris Mann’s picture

Status: Needs review » Reviewed & tested by the community

Revisions looks like it might be tricky...although it would of course mean that Drupal actually shipped with it turned on for a piece of content out of the box! I don't want to hold this patch up, since it will need some banging on to get in under the wire...let's do it without revisions and we can revisit.

@webchick: the no post info is another one that I always do as well -- gives another distinct (and visual!) difference between page and story out of the box. Hmmm...menu section expanded by default :P

@earnie: re: story description -- string freeze, plus I know there is going to be about a billion years of wrangling about exact wording. Let's not hold this up -- main point is to get that stuff in default.profile so we have a good working example that people can edit as desired. Future .x releases in 5 might allow some of these minor changes in.

@RobRoy: rawk if we can get this in...I am feeling good about this...

Setting to RTBC to get it back in queue for Dries and Neil to look at.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed this patch. It makes for a great first step. We can worry about the remaining gripes later on. Feel free to follow up after more discussion and/or fine-tuning.

Anonymous’s picture

Status: Fixed » Closed (fixed)