Skip to:
Content

Opened 2 years ago

Closed 3 months ago

Last modified 3 months ago

#6642 closed enhancement (wontfix)

BP Template Versioning

Reported by: hnla Owned by:
Milestone: Priority: normal
Severity: minor Version:
Component: Templates Keywords: dev-feedback
Cc: espellcaste@…

Description (last modified by imath)

This ticket introduces a new feature for 2.4 which provides BuddyPress with the ability to stamp it's templates with a version header allowing the checking of user overloaded templates.

Background:
This feature originally stemmed from a very brief idea I brought up on slack https://wordpress.slack.com/archives/buddypress/p1425032322000019, later I discussed at greater length the principle with imath at WCLND in March, where we both felt there was some merit in the idea. Lately with some of the great features imath has been introducing that require template adjustments it became clear this template versioning would help with these features and imath has realised the original concepts in some solid code which in it's current form is pretty much fully working , and there was a discussion in Wednesdays devchat https://wordpress.slack.com/archives/buddypress/p1443640839001147 around implementing this feature thus we are now adding this ticket.

The Benefits:
Being able to know what a templates version is greatly helps us keep users up to date with any changes we may make from time to time with templates. We can use these checks to alert the user to new features introduced and the fact that their templates will
need updating to take advantage of these features.

  • End users can keep track and have updated templates.
  • Theme designers can easily know what they need to update very quickly just by checking the settings screen.
  • Plugin developers can be confident templates will be up to date.
  • Core developers can safely build great new features.
  • Support can have a new means of establishing whether an issue is template related.

This is a big win all round!

The Implementation:
Each template has a version header.
A json file maintains our version blocks with reference to changes - changelog.json
e generate a new tab screen in BP settings to display all templates that are checked and found not to match to the current template version whereupon we notify the user which template is outdated and list the changelog features and when they were introduced.

Currently imath has produced the feature as a plugin available on his github https://github.com/imath/bp-template-checker

To avoid possible worry to non technical users seeing messages about outdated templates this feature is only activated if define('WP_DEBUG', true);

Could all core developers and anyone else interested download and run the plugin, and leave any comments and suggestions on this ticket. https://github.com/imath/bp-template-checker.git

We can have this added for the 2.4 release quite easily as most of the work is completed with really only fine detail to be looked at.

Questions RFC:

  • Is this the correct or best approach putting headers into {@internal}} inline comments - comments and thoughts on this approach are sought.

Todo:

  • Link the critical notified changes to our codex pages for code example/snippets for upgrading?
  • Need to browse trac for each template and navigate into their history to fetch changes in order to ba able to build the json changelog.
  • Each existing core template will need to have this internal version stamp added.

Lastly we need to thank imath for realising what was a concept into really solid implementation so quickly.

Attachments (6)

screenshot.png (67.7 KB) - added by hnla 2 years ago.
A view of the new BP settings screen for templates
6642.gruntlog.patch (3.1 KB) - added by imath 2 years ago.
6642.gruntlog.2.patch (4.2 KB) - added by imath 2 years ago.
6642.gruntlog.3.patch (4.7 KB) - added by imath 2 years ago.
changelog.json (121.4 KB) - added by imath 2 years ago.
6642-versions.patch (31.7 KB) - added by mercime 17 months ago.

Download all attachments as: .zip

Change History (30)

@hnla
2 years ago

A view of the new BP settings screen for templates

#1 @imath
2 years ago

So i've been working on a grunt task to do part of the job about this task :

Need to browse trac for each template and navigate into their history to fetch changes in order to ba able to build the json change log.

The tests i've made were with the https://github.com/buddypress/buddypress.git repo.

What grunt templates_changelog is doing ?

  • It "git logs" the bp-template/bp-legacy/buddypress directory
  • It tries to do a funky link between the svn revision of the commit we can find in git-svn-id ...@trunk:revision and the list of BuddyPress major release versions (i've build a script to get the revision of the commit creating the tag on svn). So it shouldn't be 100% correct, because when we work on trunk for version X.X.0, we also work on branch X - 0.1.. So this might need some eyes to check each log.
  • It builds the json object and write it in bp-template/bp-legacy/

The only pieces of information that are missing are:

  • the version when the template was introduced,
  • the importance of the change introduced (low/critical..)

I've updated the plugin https://github.com/imath/bp-template-checker so that if you use the grunt task, the plugin will use the generated changelog.json instead of the example included in it.

See 6642.gruntlog.patch

@imath
2 years ago

#2 @imath
2 years ago

Updated the gruntlog patch so that it now automatically sets the importance of template changes.
+ added an example of the generated changelog.json using the grunt task.
+ checked everything was ok with git://buddypress.git.wordpress.org/

Last edited 2 years ago by imath (previous) (diff)

#3 @imath
2 years ago

  • Description modified (diff)

#4 @imath
2 years ago

Updated the changlog.json to include changes in 6642.gruntlog.3.patch.
3.patch is adding a way to associate a codex page to a changeset.

@imath
2 years ago

#5 @boonebgorges
2 years ago

A very big +1 to the general idea. hnla does a good job of outlining the benefits.

A couple of questions and comments as I check out the suggested implementation:

  • imath's autogenerated changelog.json is very verbose. I assume that, in practice, we'd only use it as a starting point. In order for the debug information to be useful, it shouldn't be too verbose. Maybe 'low' item shouldn't be shown by default (that'd include, eg, documentation changes)
  • The changelog entries as displayed on the Templates tab are not very useful. How do we feel about linking to the changeset? Or perhaps a diff that shows the changes between the changeset and the previous commit, for the file alone. Eg https://buddypress.trac.wordpress.org/changeset?old=10180&old_path=trunk%2Fsrc%2Fbp-templates%2Fbp-legacy%2Fbuddypress%2Fgroups%2Findex.php&new=&new_path=trunk%2Fsrc%2Fbp-templates%2Fbp-legacy%2Fbuddypress%2Fgroups%2Findex.php We can easily build these links dynamically. A codex page is good too, but that's one more thing to create manually before each release.
  • I like the @internal notation. This seems less intrusive to me than the do_action() technique originally suggested.
  • Yes, this should only be enabled when WP_DEBUG. I feel pretty strongly about that. If we absolutely must notify the site admin outside of WP_DEBUG, let's do a dismissable admin notice "Hey, some of your templates are out of date. Enable WP_DEBUG to see details, or contact your theme developer."
  • Is there a way we can update/add template file @internal headers automatically? That'd be good as part of grunt precommit. Like, maybe when you commit, it leaves a timestamp or a changeset, and then part of packaging for release would involve a script that converts these timestamps/changeset numbers to version numbers. Not a dealbreaker, but it would make life a bit easier for maintainers.

In general, I think the approach here is good. I'd be OK including something like it in 2.4 if imath and hnla wanted to put in the work to complete their @todos.

#6 @DJPaul
2 years ago

Putting a version history/changelog in the template files is a good idea regardless of anything else.

From the British peanut gallery:

  • I don't think that @internal is the right phpDoc property to use. It kind of makes sense, but (I believe) anything applied at the file-level is cascaded down to classes and functions within that file. Most parsers, especially WP-Parser, will filter out @internal content when creating HTML output (or whatever) -- meaning these files' contents would just not appear in any auto-generated documentation. I think @since is better and more semantically correct. Otherwise, make up a custom tag.
  • I do not like the idea of distributing a JSON file, acting as a changelog, in BP core. One reason is that I see no reason why it couldn't just be PHP. A second reason is that a JSON file adds significant complication to our translation workflow which has not been addressed in the proposals.
  • I share Boone's concerns about being very careful to only show this to developers. I'm not convinced by all the arguments in the "the benefits" section (showing it to end users, for example, or how it will help "us" core developers in future).
  • I think the goal of what this change is trying to achieve needs to be more focused. In the screenshot on this ticket, it says "please upgrade (the) template":
    • What happens if I don't want to, or can't? Can/should I be able to dismiss the errors?
    • To help a person decide if something's important, I would want to know three things:
      • What (is the problem?)
      • Why (should I care or do something about it?)
      • How (do I make that change?)
    • Therefore, have you considered the different importance of changes? Do we present (for example) the addition of ARIA attributes in the same way that we would do if we were to make significant HTML changes? Do we need to show all messages all the time?

#7 @hnla
2 years ago

Great comments @boonebgorges & @DJPaul thanks guys - I can respond to some imath better for others.

hnla does a good job of outlining the benefits.

It must be said though with a lot of collaboration from imath in preparing ticket and bpdevel post.

Putting a version history/changelog in the template files is a good idea regardless of anything else.

In many ways this was partly the thought here, however it's eventually used / extended the principle here is a good one and if we only get all the templates docblocked for 2.4 it'll be a great start.

Yes the changelog is somewhat verbose, personally I tend to like the verbosity, the level of detail, however it's a good point in maybe not showing the minor changes.

Linking to changesets rather than changelog? yes can see the point, as for codex page I did set up a section a while back to list specific template changes by release version showing the changes required. I defer to Imath for what's reasonable or possible to link to.

let's do a dismissable admin notice "Hey, some of your templates are out of date. Enable WP_DEBUG to see details, or contact your theme developer."

Think that's a sensible idea. WP_DEBUG is definitely the switch to avoid alarming non techie users and messages if seen by them must not alarm or suggest their site is about to collapse.

Questions on docblocks are best left to yourselves to ponder I thought @internal seemed a logical choice.

how it will help "us" core developers in future

I think in the sense that we are secure in the knowledge that we're directly imparting the changes, and developers are far less likely to miss some new feature/ important change.

What happens if I don't want to, or can't? Can/should I be able to dismiss the errors?

Don't then! :) Only developers will see these so we assume they can deal with the changes. These are not necessarily errors though, however a means to dismiss them? perhaps not a bad idea, although updating their template version stamp would do this, or simply ignoring the tab, I think as it's seen vis WP_DEBUG that developers would deal with updating their overloaded templates.

How (do I make that change?)

When we have finalised where to link to they will have examples of what to change/update and we'll make things clear in those lines of text before the template version notices start.

Therefore, have you considered the different importance of changes?

Yes and no, this is a first but very strong draft if you like, we're verbose in reporting all changes and maybe that's good? Aria might not be critical or a 'feature' however not a bad thing to point out we have improved our template with? In addition note that we do highlight what changes we do consider are the ones important to attend to ( a better SC will be seen in the bpdevel post when it's published)

#8 @hnla
2 years ago

"Hey, some of your templates are out of date

I suggest we're positive here

"We have added some great new features to BuddyPress. Please check the template notices below for the templates to update to get these new features for your site"

#9 @imath
2 years ago

Thanks a lot for your feedbacks and for your reply hnla :)

This is my contribution to the debate:

Is there a way we can update/add template file @internal headers automatically?

That would be great, i agree.

GlobalIy i'd agree about the translation concern, it's so hard when you're not english to understand this kind of link http://www.phpdoc.org/docs/latest/references/phpdoc/tags/internal.html :)
But as a non english, i'm very happy to have at least some information even if it's not in my language, i mean all english is always better than 0 :)

About the keyword, honestly i have no preferences, because i don't know enough this part. i just think it might be more difficult to parse:

@since 2.3.0 added something
@since 2.4.0 added another thing
...
@since 4.9.4 ...

The advantage of

@internal {
Edited: 2.4.0
Created: 1.7.0
}}

is it's easy to update the Edited part for the people overridding the templates.

If we can use a custom keyword i'd vote for

@template {
Edited: 2.4.0
Created: 1.7.0
}}

:)

I'm not convinced by all the arguments in the "the benefits" section (showing it to end users, for example, or how it will help "us" core developers in future).

I may be naive! I thought: if we provide this kind of information, theme authors/"template overridders" will play the game (knowing end users could see it) and update their templates (Premium themes could even use it as a selling argument: my templates are up to date!), allowing end users to fully enjoy BuddyPress, letting us be confident in developing new great features, because theme authors/"template overridders" would update regularly their templates... :)

This is a first step that can evolved in the future. I think we can wait to have something perfect and never have it launched or we can make small steps and improve it at each version.

The most important is to have the templates 'stamped' with at least the information of when the template was last edited. If we could have this in 2.4 using the keyword of your choice (as soon as unique), that would be awesome.

@tw2113 (many thanks to him) did a great work adding doc blocks to each template last week-end, it would really be too bad to not fully enjoy his work in 2.4.0 because of a keyword :)

This ticket was mentioned in Slack in #buddypress by hnla. View the logs.


2 years ago

#11 @r-a-y
2 years ago

This is kind of genius, @imath.

Sorry for only reviewing this now. Here are my thoughts:

.changelog.php:

  • Marking changesets as important requires manually updating the .changelog.php file. If we are only concerned about important changesets and since we would still need to mark important changesets manually, do we need to parse every template changeset for a verbose changelog?

BP Templates Checker plugin / Template docblock:

  • Using the get_file_data() function to parse out @internal's Edited and Created version = fantastique! Very smart! :)
  • I like the use of the @internal tag. Is there a reason why two }} characters are used to close the @internal tag instead of one? Is it for parsing purposes?
  • Either @internal or @template is fine with me.
  • Themes that have previously overriden bp-legacy template parts will need to manually add the @internal header to their template parts. This is probably a stumbling block.

changelog.json:

  • I do echo DJPaul's concerns about shipping a changelog.json file into core. Couldn't we add it to the /tags/xxx/ or /branches/xxx/ repo and omit it from release? When the Templates tab fetches the changelog, it fetches the JSON file from Trac instead?
    • If we are packaging the file, perhaps format it as a HTML file instead so it functions okay if someone is just looking at the file? (Yeah, I know this is more work... Do not spend time on this! I'm just thinking out loud!)
  • I also love the automation about parsing our own internal commit logs for the changelog.json file. DJPaul mentions that this is only in English; I do not think this is a big deal if we are only exposing this via WP_DEBUG (meaning only developers would be checking this out). Localizing these commit messages would mean adding the changelog into core as a PHP file, which means more work for translators and also that this file could grow over time. So I am kind of against localizing the changelog.

This is the beginnings of a great tool, imath! I'd be okay with this in 2.4 if we can address some of these concerns.

Last edited 2 years ago by r-a-y (previous) (diff)

#12 @imath
2 years ago

Thanks a lot r-a-y for your feedback (and i forgot to mention in a previous comment, the big +1 by boonebgorges made me feel great).

I must admit i was beginning to regret the time i've spent about all this, and the time others also spent on it because of me :)

Maybe i'm the one to blame because i haven't took the time to explain what i was thinking of before actually doing it. I'm like that, i like to do things / show things. Reason is probably it's hard for me to find the right words in english to explain :)

From now on, i will think a lot more before doing things "fast & furiously" :)

About .changelog.php: my idea was to display only the important changes but also include a button to let users get all the details just in case.

About the doc block keyword @internal{}}. As i've said i'm not an expert. My idea was to find something that wouldn't bother documentation parsers, documentation experts… and use something very easy to edit for people overridding the templates who might not have any clue about doc blocks etc.

According to what i've read: @internal{}} is not Parsed by Documentation parsers, so it was fine to me :)

I thought the way WordPress does for plugin versions or theme versions, as very simple was interesting, and i was convinced WordPress was including functions to easily get these informations and found get_file_data() :) Personally and to be honest my preference would have been to have :

/**
 * BuddyPress - Activity Loop
 *
 * Edited: 2.4.0
 * Created: 1.7.0
 *
 * @package BuddyPress
 * @subpackage bp-legacy
 */

Because you're absolutely right:

Themes that have previously overriden bp-legacy template parts will need to manually add the @internal header to their template parts. This is probably a stumbling block.

If it's too complex, i'm afraid people won't update it :)

@r-a-y i've seen on internet this was the way to use @internal{}} : 2 } at the end , so i just applied :)

Side note about the keyword debate: WooCommerce is using @version.

Why json ?

  • I thought i can easily use it in php, i can easily use it in javascript
  • It's easy to build a json file with a text editor, and humans can read it

When the Templates tab fetches the changelog, it fetches the JSON file from Trac instead?

I thought we needed to include the json in Core because i mainly live inside the Intranet world and Trac might not be reachable from this world :) I wanted to make sure people using BuddyPress to power their intranets could stay easily aware of template changes.

But more globally, i agree with you :)

adding the changelog into core as a PHP file, which means more work for translators and also that this file could grow over time. So I am kind of against localizing the changelog.

I agree at 100% with your argument.

Thanks again for your feedback and your enthusiasm about it.

#13 @DJPaul
2 years ago

As far as the tags goes, use @since for "created" and @version for "lasted edited" (good call pointing to Woocommerce, imath!).

#14 @DJPaul
2 years ago

  • Milestone changed from 2.4 to 2.5

#15 @espellcaste
2 years ago

  • Cc espellcaste@… added

As far as I understood, only English info would be available. I see that as a problem. Although more work would be needed to translate those to each and every language.

If only English is allowed, or non-translated text, I can see lots of questions being asked why and how to translate it in the BuddyPress Brazilian forums.

We have a non-dev audience and most of the time, people don't speak English. Even the devs ones!

So I think this particular feature wouldn't be welcomed with a lot of joy.

But congrats on the work so far!

#16 @DJPaul
21 months ago

  • Milestone changed from 2.5 to 2.6

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


18 months ago

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


18 months ago

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


18 months ago

#20 @DJPaul
18 months ago

  • Milestone changed from 2.6 to Future Release

#21 @mercime
17 months ago

Attached patch adds @version 2.6.0 in bp-legacy files for 2.6.0 (can change to @includes x.x.x if that's preferred). Last week, I checked with @tw2113 whether there's any issue using @version x.x.x. He ran the parser on it and said "no errors or anything like that, so i think we'll be just fine".

Last edited 17 months ago by mercime (previous) (diff)

#22 @DJPaul
16 months ago

  • Component changed from Appearance - Template Parts to Templates

#23 @DJPaul
3 months ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Closing most tickets related to BP-Default and BP-Legacy, since the upcoming BP-Nouveau template pack (planned for 3.0) will make these redundant.

#24 @DJPaul
3 months ago

Closing most tickets related to BP-Default and BP-Legacy, since the upcoming BP-Nouveau template pack (planned for 3.0) will make these redundant.

Note: See TracTickets for help on using tickets.