Project

General

Profile

Actions

Feature #34863

closed

Change default text formatter for new installations from textile to common_mark

Added by Marius BĂLTEANU about 3 years ago. Updated 5 months ago.

Status:
Closed
Priority:
Normal
Category:
Text formatting
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:
Resolution:

Description

Nowadays, markdown is much more used and known compared with textile. Also, in Redmine, Markdown has better support.

My proposal is to change the default setting from textile to markdown.


Files


Related issues

Related to Redmine - Patch #35952: Explicitly specify text formatting in the test suiteClosedGo MAEDA

Actions
Related to Redmine - Feature #37365: Allow users to choose textile or markdown formattingClosed

Actions
Blocked by Redmine - Patch #36807: Remove CommonMark experimental flag and mark as deprecated the RedCarpet MarkdownClosedMarius BĂLTEANU

Actions
Actions #1

Updated by Marius BĂLTEANU about 3 years ago

  • Subject changed from Change default text formatter for new installation from textile to markdown to Change default text formatter for new installations from textile to markdown
Actions #2

Updated by Mizuki ISHIKAWA about 3 years ago

+1

Since it is not possible to change the text formatter setting for each project in Redmine, there is also the problem that it is difficult to change to Markdown once you start using it with Textile.
I think it's better to use the commonly used Markdown as the default.

Actions #3

Updated by beko akabeko about 3 years ago

+1

Markdown is used in many text systems (GitHub, Slack, ...etc). Therefore, setting this as the default will improve portability.

Actions #4

Updated by Moritz Poldrack about 3 years ago

+1

Since it is not possible to change the text formatter setting for each project in Redmine, there is also the problem that it is difficult to change to Markdown once you start using it with Textile.
I think it's better to use the commonly used Markdown as the default.

it's possible using grep, sed, and pandoc. but it's a real PITA

Actions #5

Updated by Bernhard Rohloff about 3 years ago

+1

Markdown is also the better choice for the 'average guy' and has easier to adopt by non IT people.

Actions #7

Updated by Go MAEDA almost 3 years ago

  • Target version set to 5.0.0

Setting the target version to 5.0.0.

Actions #8

Updated by Marius BĂLTEANU over 2 years ago

  • Target version changed from 5.0.0 to Candidate for next major release

I think the best option is to change to CommonMark when we will remove the experimental flag.

Actions #9

Updated by Go MAEDA about 2 years ago

  • Related to Patch #35952: Explicitly specify text formatting in the test suite added
Actions #10

Updated by Marius BĂLTEANU about 2 years ago

Now that CommonMark feature was added to the core, for sure we should not set the old Markdown as default, but at the same time, it's pity to keep textile as default.

Maybe is better to set CommonMark even is it marked as experimental? Or even remove the experimental flag?

Actions #11

Updated by Go MAEDA almost 2 years ago

  • Target version changed from Candidate for next major release to 6.0.0
Actions #12

Updated by Go MAEDA almost 2 years ago

  • Target version changed from 6.0.0 to 5.1.0
Actions #13

Updated by Go MAEDA almost 2 years ago

  • Blocked by Patch #36807: Remove CommonMark experimental flag and mark as deprecated the RedCarpet Markdown added
Actions #14

Updated by Marius BĂLTEANU almost 2 years ago

  • Subject changed from Change default text formatter for new installations from textile to markdown to Change default text formatter for new installations from textile to common_mark
Actions #15

Updated by Marius BĂLTEANU almost 2 years ago

Here is the patch to set common_mark the default formatter.

Actions #16

Updated by Holger Just almost 2 years ago

When changing the default formatter, we should ensure that the previous setting is definitely preserved. Often, the setting is stored in the database, but not necessarily. It can also be used with just the default from settings.yml. Changing the setting just there will silently change the global text formatting setting during an upgrade.

As such, I believe we need a migration here that strictly adds the current setting as a database entry before changing this (rather important) default setting. This must take several cases into account:

  • Upgrade - An existing Redmine with a text_formatting setting stored in the database
  • Upgrade - An existing Redmine with the default text_formatting from settings.yml without an explicitly stored setting
  • New Setup - An initial setup with an empty database.

A possible solution would be to perform the following two changes:

  • Add a new migration which sets the text_formatting setting in the database to the static value textile, only if there is no existing entry in the database.
    This should take care of the upgrade paths to retain the previous (default or adapted) setting regardless of how we change the default in settings.yml.
  • Update an existing old migration to explicitly set the common_mark setting to the database in the same commit as the update to the settings.yml. E.g. the source:trunk/db/migrate/017_create_settings.rb migration.
    This should take care of the initial setup. As the new default setting is set to the DB early, the later new migration will no nothing in this case. Existing installation won't re-apply the changed initial migration.

As for the timeline, please let me quote my comment from #36807#note-5:

The default formatter is only relevant for new installations. I have no strong opinion on whether this requires a major release (6.0.0) or minor release (5.1.0). I have a slight hunch that it might be surprising in a patch release (5.0.x).

Actions #17

Updated by Marius BĂLTEANU over 1 year ago

Thanks Holger for your valuable feedback, I wasn't aware that the default settings are not stored in the db from the beginning.

I think the attached patch should do the trick:

1. Setting text_formatting already stored in the database: it won't do anything and this is the expected behaviour
2. Setting text_formatting doesn't exist in the database: it will run the migration and it will persist the Setting.text_formatting in the db instead of the static textile value. That means for existing installations it will insert textile most probably (if the setting.yml file wasn't manually changed) and for new installations, it will insert @common_mark.

What do you think? Am I missing any case?

Actions #18

Updated by Jens Krämer over 1 year ago

In an existing installation without a setting in the database, when applying the patch and then running the migration, Setting.text_formatting will already return common_mark with the changed default in settings.yml.

I think the migration should set the value in the database literally to textile (as Holger suggested) in case no setting is present, to persist the previous default.

(Edit: Of course in that case Holger's second point, changing the create_settings migration in the same commit to reflect the new default for fresh installations, is necessary as well)

Actions #19

Updated by Marius BĂLTEANU over 1 year ago

You're right Jens, I didn't see this point. Thanks both for your feedback on this.

I'm going to update the patches in the following days.

Actions #20

Updated by Marius BĂLTEANU over 1 year ago

I've updated the patch, I think now it covers both cases:
  • for existing installations, it explicit save the "textile" value in the database
  • for new installations, it set during the 017_create_settings.rb migration the value to Setting.text_formatting.
  • it adds a test to assert that the default value for new installations is "common_mark".

Jens, Holger, can you take a look on the patches, please? I would like a double check before committing this.

Actions #21

Updated by Marius BĂLTEANU over 1 year ago

  • Assignee set to Marius BĂLTEANU
Actions #22

Updated by Marius BĂLTEANU over 1 year ago

  • Status changed from New to Resolved

I've committed the change, please let me know if there are any issues.

Actions #23

Updated by Go MAEDA over 1 year ago

Since CommonMark is the default formatter now, commonmarker gem should not be optional. And since Redmine already does not support JRuby, there is no need to set common_mark to optional in consideration of JRuby.

diff --git a/Gemfile b/Gemfile
index bccf76491..2d15ba953 100644
--- a/Gemfile
+++ b/Gemfile
@@ -48,11 +48,9 @@ group :markdown do
   gem 'redcarpet', '~> 3.5.1'
 end

-# Optional CommonMark support, not for JRuby
-group :common_mark do
-  gem "commonmarker", '0.23.4'
-  gem 'deckar01-task_list', '2.3.2'
-end
+# CommonMark support
+gem "commonmarker", '0.23.4'
+gem 'deckar01-task_list', '2.3.2'

 # Include database gems for the adapters found in the database
 # configuration file
Actions #24

Updated by Go MAEDA over 1 year ago

A test added in r21897 fails.

Error:
SettingTest#test_default_text_formatting_for_new_installations_is_common_mark:
NoMethodError: undefined method `value' for nil:NilClass

    assert_equal 'common_mark', Setting.find_by(:name => 'text_formatting').value
                                                                           ^^^^^^
    test/unit/setting_test.rb:151:in `test_default_text_formatting_for_new_installations_is_common_mark'

rails test test/unit/setting_test.rb:149
Actions #25

Updated by Marius BĂLTEANU over 1 year ago

  • Status changed from Resolved to Reopened
Actions #26

Updated by Ko Nagase over 1 year ago

Go MAEDA wrote:

A test added in r21897 fails.

[...]

About the above common mark test error, I confirmed that test env database doesn't have settings table record for "text_formatting" => "common_mark", while development env database has the record.

I upload settings fixture patch for test env, so reviewing it is helpful.

(Update) GitHub CI before/after results are here:
Actions #27

Updated by Holger Just about 1 year ago

  • Related to Feature #37365: Allow users to choose textile or markdown formatting added
Actions #28

Updated by Go MAEDA 5 months ago

  • Status changed from Reopened to Closed

Go MAEDA wrote in #note-24:

A test added in r21897 fails.

Fixed in r21962.

Actions

Also available in: Atom PDF