[D8] Convert "Recent content" block to a View

[quicksketch]

Followup to #40. Relevant drupal.org issue: https://drupal.org/node/2020393

The new views block should contain:

• [x] All the same fields as the node module recent content block
• [x] Operations links moved into a drop-button column
• [x] a setting for how many items to display (for BC funcionality)
• [x] a new setting for content types to display based on https://github.com/backdrop/backdrop-issues/issues/4020

Advocate: @stpaultim

[quicksketch]

This issue would be great to get handled before layouts land in #86. Currently we have to port the block settings for this block, but if it were a view, there would be no settings at all and it would simply be part of the view config.

[quicksketch]

Hm, actually a long while ago I said that maybe we should just delete this block entirely, per https://github.com/backdrop/backdrop-issues/issues/148. Thoughts?

[klonos]

I think that being able to easily add a recent content section to their site is what most new users expect out of the box from a CMS. So I'd say that converting it to a view and having it ready for them to use would be a better solution than simply getting rid of the whole thing.

[quicksketch]

Thanks for weighing in @klonos. Any thoughts on the other blocks we have? e.g. Recent users, online users, and recent comments?

[klonos]

I think they are equally useful and equally used in other CMS's. If we want to keep people coming from these happy, then we better keep them.

...were they removed from D8 BTW?

[oadaeh]

They were not removed from D8. They were converted to views.

[klonos]

Thought so...

We claim to be a CMS with D8 comparable features, so I say lets not give them any excuse to say anything for such a trivial thing.

[quicksketch]

Postponing this to a release after 1.0.0. We can still convert this at any time in a minor release.

[quicksketch]

Thanks @docwilmot for the new PR! A few issues I found:

• There's an update hook for creating the new view (great!), but we also need the update hook to loop through all Layout configurations and replace any instances of the recent content block with the new view-based version.
• The PR currently deletes the entire Node block test, it'd be preferable to keep this test intact and just update it to work with the new markup provided by the View.
• The default view configuration should be provided by node.module, not Views, so it should be moved to /core/modules/node/config.

[docwilmot]

Thanks, will do.

[docwilmot]

There's an update hook for creating the new view (great!), but we also need the update hook to loop through all Layout configurations and replace any instances of the recent content block with the new view-based version.

I can probably do this for upgrading a Backdrop site. (I think, but pointers would be welcome). But what to do about Drupal to Backdrop upgrades?

[quicksketch]

But what to do about Drupal to Backdrop upgrades?

Blocks are already converted into Layouts in system_update_1025(), so there's no need to worry about Drupal 7 block positions. You can assume that all blocks are already placed directly in layouts.

I can probably do this for upgrading a Backdrop site. (I think, but pointers would be welcome).

We could use a general install function for doing this task. The general approach is that you would load each layout config file (manually, using config_get_names_with_prefix()), then find each block that is the recent content block and replace the module/delta values with the new module (Views) and delta (the name of the view).

[Dinornis]

@docwilmot needs some imput from @quicksketch in the PR comments.

[jenlampton]

I created a new PR based on @docwilmot 's original, but added block configuration options for number of items per page, and added back the test for the number of items.

I also included a second commit that added an exposed filter (as block configuration) for node type, to address the request in https://github.com/backdrop/backdrop-issues/issues/4020. I'm also adding the feature request label to address that new feature.

Here's the front-end view of the block:

Here's what the configure form looks like for the views version:

If you have feedback, please post it here :)

[klonos]

@jenlampton the PR sandbox is broken (leads to install.php). Can you please kick it?

I've left a couple of comments in the PR. Also:

• We need the "more" link on the block to have its text be "Show more content" (and it should also be checked as an option in the "Allow settings override" settings in the view).
• Would be nice to have "new" content indicators, for consistency with how the legacy block worked/looked (this one seems to be done)
• For better UX, I would prefer the list of content types to be checkboxes instead of a select list - but I understand that this is an exposed views filter now, so this may need to be a follow-up.
• Y'all know how I feel about those nasty "leave blank to...", or "0 means all" settings 😓

[stpaultim]

I was looking for other feature requests that have PR's already and might be near completion. I like the idea for this PR, but I have the same question I had for https://github.com/backdrop/backdrop-issues/issues/4576, is this backward compatible. This PR makes visual changes to a block that is likely already in use on many sites. Are we able to do that?

It seems to me that we can do the following:

1) Add this view to new or existing sites, as long as we keep the old block (or the appearance of the old block on existing sites). 2) For existing sites, add this block but keep the old block and leave site builders with a choice 3) For new sites, add this view and block and hide the old block.

[stpaultim]

I'm considering working on this task, mostly because it looks like it might be relatively simple.

I raised the issue in DEV meeting today. The consensus from @quicksketch and @klonos was that if the block is visually different, we might be able to fix that.

They also pointed out that this view seems very specific to administrative pages as it includes edit and delete links. In which case, it might be sufficient to make sure that it looks good in the seven theme and we might be able to disregard any changes in basis.

It's my intent to create a new PR based upon the previous work of @jenlampton and @docwilmot, unless either of them want to work on this. We need a new PR to get a new sandbox and then I can test to if backwards compatibility is really an issue.

Once I've decided the scope of this issue, I may advocate for this issue and possible look at other "convert to views" issues.

[stpaultim]

I took a stab at recreating this PR since the old ones are so out-of-date.

Here is what the new block looks like placed in bottom of front page.

Compare this with the old block

[stpaultim]

I just wanted to get this far, to see if this is worth pursuing.

This PR would make visible changes to the block. The question is whether or not these are important changes or cause backwards compatibility problems, given that this view seems to primarily be an admin view.

See how this view looks to an unauthenticated user as core currently ships. It's hard to see, but there is an outline of where the edit and delete links would go if they were available to this user.

CURRENT (unauthenticated user):

The other possible compatibility problem would be that we've removed the old block with this PR. Assuming existing sites have the old block for admin purposes, can we do an update hook that would replace the old block with this one?

If we decide that this could work, but that it must look more like the original block. It's easy to move author below the node title and some of the other changes could possibly be themed to look like the old block.

What do folks think?

Is this possible? Is it worth pursuing? Or should this be a 2.x issue?

[klonos]

Thank you @stpaultim for getting this going again 🙏🏼 👍🏼

... this view seems to primarily be an admin view.

I'm not sure that we can safely assume that that would be the case most of the times. It seems like a common use case for sites to use that block in a sidebar on their home page. On the other hand, that block had so many limitations as a regular block, that I believe most sites will have already replaced it with a views-generated one. Given the usage stats of the Views module, I believe that we're probably OK.

... can we do an update hook that would replace the old block with this one?

I think that we should.

...but that it must look more like the original block.

Yup. We may not be able to match the visual aspect or the generated HTML 100%, but I would like us to try to match the old HTML tags, ids and classes.

Or should this be a 2.x issue?

No. This should have been done years ago 😅

[stpaultim]

EDIT: I had expressed some backward compatibility reservations here, but after looking at the changes I've implemented, I no longer hold these reservations and deleted them.

@klonos, Thanks for all comments on PR. I'm looking at them now.

EDIT: I've implemented all suggestions by @klonos in the updated PR.

[stpaultim]

I did a couple more things to make it look like the original view.

1) Removed Labels 2) Used Rewrite Results to put "New" tag on same line as title. 3) Removed "By" from auther. It's nice, but does not match the old block.

CURRENT VERSION OF PR:

PREVIOUS VERSION OF PR:

[klonos]

EDIT: I've implemented all suggestions by @klonos in the updated PR.

Thanks, but not quite 😅 ...some things were missed. I've added further comments with code suggestions.

I'm not sure about removing all titles/labels 🤔 ...I would rather we kept the "by" at least (even if it doesn't match the old block). I would actually like us to add some value in the new block, and add the date of creation/publishing of each piece of content. So I would prefer the second line to be "by [AUTHOR], on [DATE]". I actually think that the date is more valuable information than the author, since there's sites where the author is always the same, or needs to be omitted for anonymity (that was actually a requirement in gov sites in GovCMS-land). Anyway, I'll let others provide feedback on whether that's a nice idea or not.

My thinking for the suggestion above is that the old block provided very little value, and chances are that most sites would have used a proper, views-generated block instead. No reason to limit the usefulness that the new block will provide to new sites built on Backdrop for the sake of keeping things the way they used to be, especially for a block that chances are is not used much. I'm more concerned about making sure that we keep the old CSS classes etc. TBH.

Anyway, I like where this is going, and thanks @stpaultim for getting this rolling again. We only need an update hook, and we should be good to go I think.

[stpaultim]

Latest version:

[stpaultim]

So, it seems that for the update hook in node.install, I will need to convert the views.views.recent_content.json file into a php array. Or do I have that wrong?

If so, is there a good tool that will do this work?

config file = https://github.com/stpaultim/backdrop/blob/146-Replace-Recent-Content-Block/core/modules/node/config/views.view.recent_content.json

Replace PHP array in:

https://github.com/stpaultim/backdrop/blob/146-Replace-Recent-Content-Block/core/modules/node/node.install function node_update_1023()

How do folks do this? Or am I going in wrong direction?

[argiepiano]

@stpaultim do you still need help with that last question? It looks like this is done correctly in your PR.

[stpaultim]

@argiepiano I do still need help.

I originally rebased a PR from Jen Lampton that already hadb that code for the node.install file. Since then, I've made changes to the view. I was hoping to simply cut and paste the json from the view into the install file, but it needs to be converted to PHP. Do I have to figure out where the changes in the view are in the PHP array and make the changes manually? :-(

Truthfully, I'm not entirely certain why the install file can't simply load the config file.

@jenlampton

[argiepiano]

I see.

Yes, if the view has changed, you have to convert the JSON into a PHP array, and replace the current array in the install file.

This is fairly simple:

1. I assume you already have that fixed view in your test site.
2. Install devel
3. Run the following code in devel:

$c = config('views.view.recent_content');
var_export($c->get());

This will output the array version of the json code. Simply copy the whole array from the output, and replace the old array in the install file.

And the reason this is needed, is that, this new view will not be installed automatically in already existing Backdrop installations - only on new installations. Therefore this view needs to be installed "manually" in the update hook for existing sites.

[EDIT]: you may need to do a bit of manual beautifying of the output to conform with style guidelines. For example, var_export inserts a return after the double arrow of 'page' => array(...

[stpaultim]

@argiepiano This is exactly what I was looking for. Yes, I have a copy of the view. In fact, I've already updated the PR with the config file for the view, but I haven't updated the install file yet.

I'll try this ASAP.

[stpaultim]

FYI - I'm thinking of working on this for 1.29. If I can get the JSON file converted to PHP, I think the rest is pretty straight-forward and has been well discussed in the past. If anyone deems this too complicated for a last minute feature, I'd be ok delaying it. But, the pressure of deadline helps me focus, so I use that to work on this PR either way later today.

[stpaultim]

I think we're ready for testing and code review again.

I could use some fresh eyes on this.

Looking through the history of this issue. These are all items that were raised at some point or another:

• [ ] we also need the update hook to loop through all Layout configurations and replace any instances of the recent content block with the new view-based version.
• [ ] The PR currently deletes the entire Node block test, it'd be preferable to keep this test intact and just update it to work with the new markup provided by the View.
• [ ] The default view configuration should be provided by node.module, not Views, so it should be moved to /core/modules/node/config.
• [ ] but added block configuration options for number of items per page,
• [ ] added back the test for the number of items.
• [ ] I also included a second commit that added an exposed filter (as block configuration) for node type, to address the request in https://github.com/backdrop/backdrop-issues/issues/4020. I'm also adding the feature request label to address that new feature.
• [ ] We need the "more" link on the block to have its text be "Show more content" (and it should also be checked as an option in the "Allow settings override" settings in the view).
• [ ] So I would prefer the second line to be "by [AUTHOR], on [DATE]"

[stpaultim]

One thing I have not tested yet and that NEEDS testing is updating an existing site that is using the old blocks.

[stpaultim]

The update for this is not working. I'm working on updating the PR right now.

[stpaultim]

I placed a recent content block on the home page of a site using the most recent dev branch of Backdrop CMS. I set it to show the 3 most recent posts.

I then upgraded the site to use this PR and this is what I got. Note, it's not updating the block settings for the previously set number of posts. Given that other changes to this ADMIN block are also happening, I'm not sure if this is a blocker.

[stpaultim]

Other than the issue raised in the previous issue, this seems to be working for me. I would really appreciate someone else taking a look, testing, and peeking at the code to check for problems.

@argiepiano Thanks for that tip back in https://github.com/backdrop/backdrop-issues/issues/146#issuecomment-1859048294. I finally got around to using it.

[quicksketch]

I did a code review in the PR: https://github.com/backdrop/backdrop/pull/4596

A few suggestions:

1. Let's add <div class="node-title"> and <div class="node-author"> wrappers in the view configuration to match the previous markup.
2. If we're going to include the date in the author information, it should IMO match the default "submitted by" format, which is [node:created:medium] by [node:author].
3. In the update hook, we should add a block class of block-node-recent to match the previous class. We do not need this same class when placing the block on new sites (users can add it if really desired when placing).
4. There are still a number of code style issues caught by phpcs.
5. Tests are not yet passing.

[stpaultim]

In the update hook, we should add a block class of block-node-recent to match the previous class. We do not need this same class when placing the block on new sites (users can add it if really desired when placing).

How do I do this?

[stpaultim]

@quicksketch

1 and 2 are done.

Not yet sure how to do 3.

Looking at 4 and 5 now.

[quicksketch]

@stpaultim I pushed a commit to the PR that makes two change to the update hook:

• The "node_count" setting should now be carried over to the view block "items_per_page". You can confirm this by setting the number of nodes in the block before the update.
• The "node-recent-content" class is now added to existing blocks as part of the update hook.

[jenlampton]

I've tested the upgrade path, and the transition is pretty seamless.

• the class block-node-recent remains on the block
• The classes node-title and node-author are applied on the same rows
• The number of items remains as a block setting
• Same actions are present
• The table styles remain the same (seems kinda broken, but unrelated to this issue)

The only significant difference is that before, where it had only the author name in the second line, it now has the author name as part of the submitted by information (see below). In general, I think this change is an improvement, and I think the improvement outweighs the fact that it's a change.

Before:

After:

Trivial nit: It's a little weird that the class applied to the submitted info is still node-author even though it's now submitted by information. I'd almost rather we use the submitted class instead, or also? <footer><p class="submitted">.

"Submitted by" format is slightly different than it is on a node (by default)

• On a node: Mon, 09/02/2024 - 8:18pm by root -- not linked
• This block: Mon, 09/02/2024 - 8:18pm by [root]() -- linked

I also think having the author linked in this instance is valuable, and outweighs the fact that it doesn't match the node submitted by format exactly.

[jenlampton]

I just noticed something, we are missing the "More" link from D7.

It looks like the views setting is there, but that setting only adds a link if there is both a Page and a Block display of the same view. Here, recent_content seems to be it's own view instead of a second display of node_admin_content, so that More link setting won't do anything.

I recommend that we remove the More link setting, rather than moving the block display to the admin/content view.

[jenlampton]

I have done a code review, and I think everything looks good except for 1) the missing @deprecated in drupal.inc and 2) removing the more link settings from the new recent_content view.

[stpaultim]

I made an effort tonight to get this through. BUT, there are a couple of test failuers that I just can't figure out and I have to quit.

Also, because I was focused on tests, I never got to item #2 in the notes from @jenlampton - see previous comment.

If anyone wants to help push this across the finish line. I would appreciate it. However, this feature has waited 29 releases. It's not the end of the world if it waits one more.

[jenlampton]

I worked on this, and @quicksketch also worked on this, but I dont think it's going to get there tonight. It is really close though! thank you @stpaultim

[stpaultim]

Postponing this to a release after 1.0.0. We can still convert this at any time in a minor release.

@quicksketch was very accurate when he said this would get into a release AFTER 1.0. https://github.com/backdrop/backdrop-issues/issues/146#issuecomment-70041987

[quicksketch]

I'm taking another look at this to see if we can get the tests passing.

[quicksketch]

Tests correctly identified several issue with our upgrade path. I corrected the issues in the update hook and tests and we're now totally passing! :tada:

It's been a while since this was updated with 1.x, so I merged in the latest. This could use another round of reviewing.