Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[UX] Make sure that versions shown in the Modules page are as consistent as possible #5002

Open
alanmels opened this issue Mar 16, 2021 · 44 comments · May be fixed by backdrop/backdrop#3567
Open

Comments

@alanmels
Copy link

alanmels commented Mar 16, 2021

All is good for the core modules on the Modules page (admin/modules) as their versions always contain the Backdrop's version first and the module version after the hyphen, for example 1.19.x-dev if you pulled the development branch from github.

As for the contributed modules, most of them show versions like, for example:

Automated Logout  1.x-4.5.4 
API  1.x-1.10-beta2 
API Search Integration 1.x-1.10-beta2
Coder Review  1.x-1.0.1-beta 

where, as I understand, the first part - 1.x - is the main Backdrop version and the module version after the hyphen.

But some module versions get really messy.

For example, if you enable the Forum module, then it shows 7.x, and that's totally the module's fault and I reported it on backdrop-contrib/forum#39.

However, what about modules like, for example, Better Exposed Filters that shows 3.x-dev or BUEditor that shows 2.x-dev? The system seems to be taking the last part of the default branch and add -dev part. Is that really how should it be or the first part of any module before the hyphen sign should always indicate the main Backdrop version?

Interestingly, if you use a downloaded zip version, for example, https://github.com/backdrop-contrib/better_exposed_filters/archive/1.x-3.x.zip and extract, then the Modules page shows empty for the version.

@alanmels
Copy link
Author

alanmels commented Mar 16, 2021

According to @indigoxela (https://backdrop.zulipchat.com/#narrow/stream/218635-Backdrop/topic/Version.20format.20for.20the.20modules) it should be semver:

@indigoxela: It's semver..., for instance 1.x-1.0.1 (core-major.minor.patch).
Hm... It's the module's major version...
Obviously it is a bit ambiguous, as we both had to think for a moment, if that's correct. :g

But versions displayed like 2.x-dev or 3.x-dev are not following Semantic Versioning as they show only minor versions (the second part of their branch names).

Please re-tag this as a bug and I will create PR.

And if this will be re-tagged as a bug, then the question remains should the Modules page display versions for the contributed modules in form of core-major.minor.patch or should they contain information only for module itself in form of major.minor.patch without indicating Backdrop core?

@alanmels alanmels changed the title Version format for the contributed modules on the Modules page The Modules page should follow Semantic Versioning when displaying module versions Mar 16, 2021
@alanmels
Copy link
Author

alanmels commented Mar 16, 2021

Also, I'm just wondering while for the official releases we are showing versions like 1.x-1.0.0, 1.x-1.2.0 wouldn't be displaying versions like 1.x-1.x, 1.x-2.x for development branches just enough? Why the system has to also add the -dev suffix and displaying them as 1.x-1.x-dev, 1.x-2.x-dev instead?

@indigoxela
Copy link
Member

indigoxela commented Mar 16, 2021

Agreed, there seem to be some inconsistencies in version display. Never paid attention so far, but it caused me some head scratching, too.

Here's a screenshot to clarify:

inconsistent-version-display

What baffled me the most, was that dev module versions don't show the core version, while stable releases do.
I probably never realized, because none of my own modules/themes has reached a 2.x or 3.x major version, yet. 😉

I find this really confusing:

  • 3.x-dev (wouldn't 1.x-3.x-dev be less misleading?)
  • 1.x-3.0.1
  • 1.x-2.1.2
  • 1.x-dev (1.x-1.x-dev?)
  • 1.18.1 (hm... tricky. Core...)

@indigoxela
Copy link
Member

Regarding the zip snapshots, that currently show no info re version at all - wouldn't unknown be friendlier? Or 1.x-unknown - as we do know the core version, which has to be included in the info file.

@alanmels
Copy link
Author

So you think the core should be included, right? If so, then following the core-major.minor.patch format, some modules like, for example, Better Exposed Filters, will display 1.x-1.x.3.x-dev. Does that look right to you? What if we shorten versions like that to just 1.x-1.x.3.x?

@alanmels
Copy link
Author

alanmels commented Mar 16, 2021

Also what is core version exactly? Is it really 1.x or as the prefix for the current dev branch shows 1.19.x? I mean if the contrib modules are prefixed with 1.x then the core modules are prefixed with 1.19.x-dev and the difference between core and contrib modules also would look weird.

@indigoxela
Copy link
Member

indigoxela commented Mar 16, 2021

1.x-1.x.3.x-dev ugh... that duplicates the core version. Shouldn't it rather be 1.x-3.x for consistency?

I find both an improvement:

  • 1.x-3.x
  • 1.x-3.x-dev

Currently I'm slightly leaning towards the second variant, as it explicitly mentions that it's a dev version.

@alanmels
Copy link
Author

alanmels commented Mar 16, 2021

1.x-1.x.3.x-dev ugh... that duplicates the core version. Shouldn't it rather be 1.x-3.x for consistency?

Right, but 1.x is coming from how a contrib module decided to write the first part. Someone could easily create a branch 2.x not following Backdrop core compatibility requirement. Maybe, using this opportunity we should explicitly indicate the core version for contrib modules not leaving them any room to write incorrect prefixes?

Also note that while all contrib modules will have 1.x prefixes the core modules do 1.19.x prefixes (if core downloaded with git clone), which also looks inconsistent.

Currently I'm slightly leaning towards the second variant, as it explicitly mentions that it's a dev version.

Well, to say frankly me too, but then I realize that's a matter of habit to see that way. Aren't all modules with versions ending with .x are development branches anyway?!

@klonos
Copy link
Member

klonos commented Mar 16, 2021

1.x-1.x.3.x-dev doesn't seem link correct semver to me. If a contrib module has branches like the following:

  • 1.x-1.x
  • 1.x-2.x
  • ...

Then it will have it's versions have this format:

  • 1.x-1.x.y
  • 1.x-2.x.y
  • ...

...with the actual versions being:

  • for the 1.x-1.x branch:
    • 1.x-1.0.0
    • 1.x-1.0.1
    • 1.x-1.0.2
    • ...
    • 1.x-1.1.0
    • 1.x-1.1.1
    • 1.x-1.1.2
    • ...
    • 1.x-1.2.0
    • ...
  • for the 1.x-2.x branch:
    • 1.x-2.0.0
    • 1.x-2.0.1
    • 1.x-2.0.2
    • ...
    • 1.x-2.1.0
    • 1.x-2.1.1
    • 1.x-2.1.2
    • ...
    • 1.x-2.2.0
    • ...

So I would have the dev versions for each branch be like so:

  • 1.x-1.x-dev
  • 1.x-2.x-dev

NOT like so:

  • 1.x-1.x.1.x-dev
  • 1.x-1.x.2.x-dev
  • 1.x-1.x.3.x-dev

@alanmels
Copy link
Author

@klonos, thanks for the input, but that part is more or less clear. Could you please share what you think should we leave the -dev suffix when it's clear that anything that ends with .x is dev branch anyway?

@olafgrabienski
Copy link

Aren't all modules with versions ending with .x are development branches anyway?

I guess that's clear for developers but not for site builders, at least I didn't know it.

@klonos
Copy link
Member

klonos commented Mar 16, 2021

...but to the point of this issue: yes, we should have consistency 👍

Do you guys think that there's point in having the core version 1.x- be prepended to every single contrib module and submodule version that doesn't have it, or remove it altogether and keep the simplified format of major.minor.bugfix?

Could you please share what you think should we leave the -dev suffix when it's clear that anything that ends with .x is dev branch anyway?

To be frank, I'm for simplicity and "economizing" on the width of the version column on that page, to reserve it for more useful information. But then I saw @olafgrabienski's comment and do understand his point.

The reasoning for the -dev suffix is to provide some sort of consistency with other suffixes, like -beta1 or -RC3 etc. Think of these examples:

  • 1.x-1.3.3-beta
  • 1.x-2.1.7-alpha
  • 1.x-3.3.0-beta3
  • 1.x-4.0.0-RC5
  • 1.x-1.x-dev
  • 1.x-2.x-dev

Although... as you may notice from the above examples, the difference is that:

  • suffixes like -beta1 and -RC3 get added to actual module releases (versions with minor.major.bugfix numbers)
  • the -dev suffix gets added to "branch" versions (things that you download directly from the GitHub branch, which only has minor.major numbers)
  • versions with the -dev suffix always have x for their major number
  • versions with the -dev suffix have no .minor digit

To the last point, we should keep in mind the discussions is #664, which may result in dev versions like these:

  • 1.x-1.1.3+5-dev (where 5 is the number of commits after 1.1.3)
  • 1.x-1.1.3+20210316235959-dev (where 20210316 is the YYYYMMDD date and 235959 is the HHMMSS time of the last commit to 1.x)

@klonos klonos changed the title The Modules page should follow Semantic Versioning when displaying module versions [UX] Make sure that versions shown in the Modules page are as consistent as possible Mar 16, 2021
@alanmels
Copy link
Author

alanmels commented Mar 16, 2021

Right, as @klonos noticed all those betas, alphas added to the module releases, and development versions always end with just .x. So for brevity we could start using just 1.x-1.x format and you will see we all, including site builders, quickly get used. Seeing the -dev suffix is just a matter of habit.

@klonos
Copy link
Member

klonos commented Mar 16, 2021

Thanks for providing a PR @alanmels ...I'll do some testing and provide some before/after screenshots, to help us decide, and move this forward.

@indigoxela
Copy link
Member

indigoxela commented Mar 16, 2021

To the last point, we should keep in mind the discussions is #664, which may result in dev versions like these:

1.x-1.1.3+5-dev (where 5 is the number of commits after 1.1.3)
1.x-1.1.3+20210316235959-dev (where 20210316 is the YYYYMMDD date and 235959 is the HHMMSS time of the last commit to 1.x)

Interesting issue, but I'm too lazy to thoroughly read it now. To be honest, this detailed information is mainly useful for developers, but doesn't provide much for admins. And there's already an issue for that, so I don't think we have to cover that here.

@klonos Or do you think, that the other issue might conflict at some point with this one?

@klonos
Copy link
Member

klonos commented Mar 16, 2021

@klonos Or do you think, that the other issue might conflict at some point with this one?

Both issues touch on the -dev suffix in module versions, so yeah, that ^^ ...I would need to thoroughly test @alanmels' PR before I am able to say more though, as I would need to try and think of all the various scenarios (that I can think of).

#664 is specifically about having some way to distinguish that a -dev version is older or newer than another one. Consider this scenario:

  • You currently use version 1.x-1.2.3 of a module, which has a known bug.
  • The bug is fixed and committed, but no new (1.2.4) release of the module is yet created.
  • You manually download from GitHub, so you now have 1.x-1.x-dev (with the fix).

The bug that your site had is now fixed, but the core Update module now has absolutely no way to let you know when version 1.2.4 of the module is released (no way to tell if that version is newer than the 1.x-1.x-dev you currently have installed).

Another issue/side-effect is this:

  • Your site has 1.x-1.x-dev with the fix for the issue you were previously having
  • Another few bug fixes happen, so more commits in the 1.x-1.x branch, but since there is no actual release, there is no way to tell if the 1.x-1.x-dev installed on your site is newer/older than the 1.x-1.x-dev on GitHub. So once again, the Update module has no way to notify you of newer releases.

Drupal has figured those things out, and dev versions are numbered. This is not just a core issue - it is also tied to infrastructure (GitHub, and how we could possibly version "dev releases").

So while #664 tries to figure out how to deal with those issues, this issue here is about making "dev" versions consistent when listing modules - so this is a "visual" display issue rather than a practical/logic issue. What I want us to be careful with here is to not make things more complicated and make #664 harder to solve.

@indigoxela
Copy link
Member

Both issues touch on the -dev suffix in module versions,...

I see. Actually I feel like we should keep the "-dev" suffix as-is for now. People are used to it, and as @olafgrabienski stated, it's more obvious that it's a development version, when the suffix is there. So the change here would only be to not strip the core version for consistency with stable version display. - And display something when the version can not be determined (like for zip snapshots).

What I want us to be careful with here is to not make things more complicated and make #664 harder to solve.

That's understood. IMO that's not the case, as the change here is minimal and - as you already stated - is a display issue, not a change in the underlying logic.

@indigoxela
Copy link
Member

However, I'm afraid the patch branch was created on top of https://github.com/alanmels/backdrop which is not up-to-date.

@alanmels Github is smart enough to not add changes that don't belong to the PR. When merging, only these changes will be considered:
https://github.com/backdrop/backdrop/pull/3567/files

While it's a good idea to keep your fork up-to-date with upstream, it does no harm if it's not. As long as your PR has no conflicts, everything should be fine.

@philsward
Copy link

UI looks good to me. Very familiar coming from D7 and a welcome no-doubt to folks working on testing PRs. (Assuming that's what the dev means... Didn't read the full issue 🤷‍♂️)

@alanmels
Copy link
Author

However, I'm afraid the patch branch was created on top of https://github.com/alanmels/backdrop which is not up-to-date.

@alanmels Github is smart enough to not add changes that don't belong to the PR. When merging, only these changes will be considered:
https://github.com/backdrop/backdrop/pull/3567/files

While it's a good idea to keep your fork up-to-date with upstream, it does no harm if it's not. As long as your PR has no conflicts, everything should be fine.

Good to know! Thanks.

@klonos
Copy link
Member

klonos commented Mar 20, 2021

@klonos mind to share your thoughts?

This looks more consistent now 👍🏼

@philsward
Copy link

philsward commented Mar 20, 2021

Finally read (skimmed) the issue and I have some thoughts about things I didn't notice before. They aren't blockers to this issue by any means, just some observations from an "outsider" as possible UX enhancements.

  1. it might be confusing to new folks, even coming from Drupal to have the 1.x-x.x . I don't have a problem with it being there but I don't feel like there is enough context of why there is a 1.x to be easy to understand at first glance. I gained context by reading this issue. Others won't have this luxury. I would suggest either create a separator between the core version and module version, or add some help text at the top that explains the versioning (or a link to a help page), or both.

Developers might pick up on it quickly, but non-developers may not be so quick.

  1. Thoughts on adding a PR number to the dev versions? It could even be a second line underneath the version. It might be a welcome addition to devs who pull a PR to know which one they pulled.

This idea could be started as a new issue and addressed as a QoL at a later time.

@ghost
Copy link

ghost commented Mar 20, 2021

There's no chance of one module being 1.x while another is 2.x (after Backdrop v2 is released) is there? In which case why have 1.x there at all? That's like adding 'module' as a prefix to all of them. Unnecessary IMO...

@klonos
Copy link
Member

klonos commented Mar 20, 2021

Thoughts on adding a PR number to the dev versions? ...This idea could be started as a new issue...

Already an existing issue: #664 😉 ...although not "PR number", rather than commit number.

@quicksketch
Copy link
Member

I would have a slight preference for actually removing the 1.x- from all versions on the modules page, rather than add it back as the current PR is doing, because for end-users I think the 1.x- prefix is confusing because it doesn't match the versioning of pretty much any other software in existence except Drupal, which is moving away from that and just using Semver now.

However, the good reason to keep it though is that if we were to encourage straight Semver usage (no more prefixing with the core version compatibility), then including the prefix makes it clear which modules are using Semver and which ones are not.

Backdrop core does not care if you have a major version prefix or not. You can have a module be version 1.x-1.0.0 or just 1.0.0. Similar you can have a 1.x-1.x-dev version or just 1.x-dev (with a Git branch of 1.x). Backdrop does not strictly enforce the prefix or Semver, right now it can be either. The Backdrop contrib process encourages using 1.x- prefixes currently, that's really the only policy we have in place.

All that is to say, I think the current PR is fine. As long as we go entirely one way or the other (either hiding 1.x- for everything or showing 1.x- for everything), it's an improvement.

@alanmels
Copy link
Author

alanmels commented Mar 24, 2021

Thanks Nate for the details. I'm really neutral on this, but practicality in me leans toward not having 1.x- prefixes. What do you all, guys, think?

@ghost
Copy link

ghost commented Mar 24, 2021

As I said above:

In which case why have 1.x there at all? [...] Unnecessary IMO...

So I vote for removing it.

@indigoxela
Copy link
Member

One tiny concern: if the '1.x' prefix isn't displayed, then core and contrib aren't as easy to distinguish anymore.

We'd have:

  • 1.18.1 (stable core)
  • 3.0.1
  • 3.x-dev
  • 1.18.1 (some contrib on that version)

Or with dev core:

  • 1.19.x-dev (dev core)
  • 1.x-dev
  • ...

And then an edge case, if you have a D7 module, which isn't ported yet:

  • 7.x-2.x-dev

The main thing seems consistency, though. Either always strip or always show. 👍

@klonos
Copy link
Member

klonos commented Mar 26, 2021

One tiny concern: if the '1.x' prefix isn't displayed, then core and contrib aren't as easy to distinguish anymore.

I'd rather we implemented #498 to address this concern.

And then an edge case, if you have a D7 module, which isn't ported yet

So the situation with the PR is that the 1.x- prefix (BACKDROP_CORE_COMPATIBILITY) always gets added to the version number to anything that is not core. That prefix is fine for the actual module data, so any logic that depends on that shouldn't change, (system_rebuild_module_data() or _system_rebuild_module_data() should not be touched for example). But I really do not see any real value in showing that prefix in the "Version" column, when everything in the module list is 1.x. Besides, I don't think that we are going to have anything prefixed with 2.x- till Jan 2025 ...and even then, we will need to adjust the major version logic to allow multi-compatibility (for modules that are able to work in both 1.x and 2.x).

How about we do the following?:

  • hide the 1.x prefixes from the "Version" table column when all modules in the list are 1.x-compatible, as that offers very little value in that case (plus it wastes horizontal space)
  • show the prefixes if there are multiple major compatibilities, such as a mix of 1.x/2.x/7.x for example (in that case, there is value to show the major version)

The main thing seems consistency, though. Either always strip or always show. 👍

Yup 👍🏼 ...all that "hide/show major version prefix" logic should only happen in system_modules() and theme_system_modules_fieldset(). So for display purposes only.

I filed a PR against @alanmels' PR here: alanmels/backdrop#1 (there are many changes because @alanmels's fork is behind - the actual commit with my changes is alanmels/backdrop@0fac5fb)

If only 1.x modules exist, the major version prefix is hidden:

image

...the assumption is that this is the 80% use case, so people will see "simplified" versions of modules.

If a mix of major versions exist, the major version prefix is shown:

image

...the assumption here is that this is a scenario for developers and people experimenting on their local. In that case, showing the major version prefix actually helps.

Noting that in real scenarios, any 7.x module present would be a temporary situation. These modules should eventually be either ported, or removed from the site. When that happens, the prefixes will once again be hidden from the version column.

@alanmels
Copy link
Author

I think Greg found perfect solution to address all concerns expressed on this page. I've just merged your PR, @klonos, thanks a lot!

@ghost
Copy link

ghost commented Mar 26, 2021

When that happens, the prefixes will once again be hidden from the version column.

Doesn't that go against the whole 'consistency' thing...? If someone sees the non-prefixed versions as default, then installs a D7 module, then they start seeing prefixes on Backdrop modules that didn't have one before, and core modules still don't have prefixes, that seems worse IMO...

@ghost
Copy link

ghost commented Mar 26, 2021

I'd prefer it if prefixes were removed from all Backdrop modules, period. Leave them on Drupal modules to make it obvious they're different.

@philsward
Copy link

philsward commented Mar 27, 2021

My only thoughts at this point is maybe add -core to the end of all core modules.

It isn't obvious looking at the number if it's a core or contrib at first glance and the core numbers tend to blend with the contrib numbers that are close to core. Example 1.18.2 vs 1.8.0.

It's totally usable in it's current form, just slower to wrap your head around and digest.

@klonos
Copy link
Member

klonos commented Mar 27, 2021

I'd prefer it if prefixes were removed from all Backdrop modules, period.

I hear you @BWPanda. That's my preference as well 👍🏼 ...but in the case of "mixed major versions", showing no major version compatibility prefixes is confusing, and it becomes a necessity. The two scenarios I could think of where this becomes an issue is Backdrop 2.x, and Drupal 7.x.

  • Backdrop 2.x: It will become an issue when the Backdrop 2.x branch is created (although, as I already said, that's many years from now), unless we specifically make it so that 1.x modules do not work with 2.x (which I personally think would be a wrong move).

  • Drupal 7.x: this is a problem now, and it will be for a few more years, while D7 sites are moving to Backdrop. Won't be an issue at all eventually (but still nicely handled in our code).

The PR as is (after @alanmels has merged my PR) does the following:

  • Simplifies version numbers shown by default on the modules listing page (happens in the "80%" use case, which is Backdrop 1.x sites with only 1.x modules). This is what most people using Backdrop will experience.
  • Shows major version prefixes only in cases where they are helpful. Once the reason for showing the major version prefixes goes away (incompatible modules either completely removed from the site, or replaced with properly ported ones), things fall back to the "80%" scenario. This "feature" is of temporary nature, and serves as a means of indicating a situation. It is there to help site builders/admins when upgrading from D7, as well as module developers that are porting modules over.

What this PR does NOT do (which should be a separate issue, when it starts becoming a problem for us):

  • It does not deal with Backdrop "mixed compatibility", which is the case where 1.x modules will still work as expected with both Backdrop 1.x, as well as 2.x. Our Drupal brethren have solved that problem by "decoupling" the core: property in .info.yml files, and at the same time introducing a new core_version_requirement: one, which can accept multiple core version numbers. See: https://www.drupal.org/node/3070687

    The new core_version_requirement key in *.info.yml files for modules, themes, and profiles now supports semantic versioning as implemented by the Composer project. This allows modules, themes, and profiles to also specify that they are compatible with multiple major versions of Drupal core.

    For example a module that is compatible with Drupal 8 and Drupal 9 can have a info.yml file like this

    name: My Module
    type: module
    core: 8.x
    core_version_requirement: ^8 || ^9

    This specifies that the module is compatible with all versions of Drupal 8 and 9. The core: is required here because Drupal Core versions before 8.7.7 do not recognize the core_version_requirement: key.

  • It does not deal with the "edge case" where a module could be compatible with both D7 as well as Backdrop 1.x. I think that our Drupal compatibility layer does a good job with that (I have not heard otherwise at least).

@klonos
Copy link
Member

klonos commented Mar 27, 2021

My only thoughts at this point is maybe add -core to the end of all core modules.

@philsward again, that can be solved by #498 ...if we added -core at the and of every core module, we'd end up with versions like 1.19.x-dev-core, which is too lengthy.

@indigoxela
Copy link
Member

I see some disagreement going on here. Does this mean, that the RTBC label should be removed?

@klonos
Copy link
Member

klonos commented Mar 27, 2021

The label should be removed because the PR code has changed (considerably) - not because of disagreement 😅

@philsward
Copy link

Why not add another column to show the compatible core version? Outside of making the table a bit more cluttered, it would at least solve the problem of showing the core version the module was written for without making the modules version hard to understand.

Like mentioned before, it can probably eventually go away in future versions of core.

@alanmels
Copy link
Author

@philsward adding another column (when actually the whole idea was about to also save some horizontal space) for only those rare cases when D7 module are listed, would be overkill. I think the changes @klonos brought with last PR merged are just perfect, because most of the time users will see only modules versions, and only those rare cases when they really need to be wary about version differences - like in case with D7 - they will see core versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment