helper_alternate_class() method removed. I expect this to be popular.

Known issue. That actually caused a LOT of display issues not only with plugins but also within Mantis core itself, I probably authored 20+ commits to fix these.

Basically, instead of relying on helper_alternate_class(), the table/form should be assigned the appropriate container class, allowing automatic row color alternation through CSS [1]

[1] https://github.com/mantisbt/mantisbt/blob/master/css/default.css#L543

I understand the css approach to do it, which is now the standard for MantisBT 1.3 codebase. However, I wonder if we should keep the method in the code in order not to break the plugins? We could remove this method when the new db layer gets checked in. i.e. do one round of breaking changes in one release.

I wonder if we should keep the method in the code

I actually thought about that when I started hitting HTML/CSS issues when testing master, then I realized that the fixes were generally simple enough that it was probably not worth putting the function back in and went for fixing all occurences I could find instead.

My thinking was that the time between 1.3.0a1 and 1.3.0 go live would be sufficient for plugin authors to fix their code.

We could remove this method when the new db layer gets checked in. i.e. do one round of breaking changes in one release.

I take your point about making it a smoother upgrade experience for instances relying on 3rd party plugins using that functionality.

However, if we do reinstate the helper_alternate_class() function in 1.3 I think we should at least have the function trigger some kind of warning or notice to inform the plugin's authors they are now relying on a deprecated feature.

I would also do a quick scan of the plugins hosted on github to see how many actually do rely on this feature and notify their authors, to proactively identify as many issues as possible.

There is a commit that breaks sending email if notification is triggered by SOAP or triggered by a plugin that runs through cli (e.g. https://github.com/mantisbt-plugins/EmailReporting)
See discussion at
https://github.com/mantisbt/mantisbt/commit/cea405ccf228fd2c6ac694574a74e87396b14f1f

The SOAP issue might be fixed with this PR which is not committed until now
https://github.com/mantisbt/mantisbt/pull/113

@atrol re: 0017437:0040820 - FYI, I merged that pull request a few days back.

Following up on my own comment in 0017437:0040810 "the function (should) trigger some kind of warning or notice to inform the plugin's authors they are now relying on a deprecated feature."

I did a quick test, and would propose the following approach

1. Restore the helper_alternate_class() function (i.e. partial revert of https://github.com/mantisbt/mantisbt/commit/6aa5e9b81fb2fbd2bf0ad46b416d5f1445c7f98a)
2. add the following code to the function

   
   
   error_parameters( FUNCTION, 'CSS' );
   
   trigger_error( ERROR_DEPRECATED_SUPERSEDED, WARNING );
   
   

Based on my testing, this allows backwards-compatible behavior for plugins that have not yet adopted the new CSS-based approach (i.e. correct display with alternating row colors), when $g_display_error[E_USER_WARNING] = DISPLAY_ERROR_NONE

When $g_display_error[E_USER_WARNING] = DISPLAY_ERROR_INLINE (which is the recommended setting for developers), the page will render incorrectly (i.e. table rows will have a white background) and warnings will be displayed, as shown in attached screenshot [alternate_class_warning.png]. This is due to the error message being printed inside the <tr> tag.

The only issue I can think of with this approach is that DISPLAY_ERROR_INLINE is actually the default value in config_defaults_inc.php.

As a side note, I'm wondering if we should maybe change this - IMO the defaults should generally reflect what an admin would want to use in Production; based on that, warnings should probably be set to DISPLAY_ERROR_NONE. What do you think ?

An alternative solution to point 2 above, would be to capture use of the deprecated function in a variable instead, and trigger a single warning at page bottom (maybe in html_page_bottom1a()).

Which approach do you think is best ?

With regards to usage, a quick grep finds 84 calls to helper_alternate_class() in the plugins hosted on Github (not counting the source integration plugin which I'm in the process of fixing).

I agree that we should make the error reporting defaults reflect what an admin would want to use. We could apply dev defaults if host url is localhost or 127.0.0.1.

I personally wouldn't worry too much about bring this to the attention of the developer, since I expect that this will end up having an annoying effect on others who leverage the same plugins on their dev machine. In this version we will make it work. In the next version, we will break plugin, and without the plugins doing work, they will be broken anyway.

An alternative approach is to decide to break the plugins and use the beta period as the time for plugins to be updated. We just need a way to manage users upgrading MantisBT in place while plugins are already installed. Maybe disable them if they are not marked as compatible with 1.3? Just thought I would put this also on the table. This approach will also be useful with the next release.

will end up having an annoying effect

Which might hopefully push the annoyed developer to patch the offending plugin and provide a pull request ;-)

In this version we will make it work. In the next version, we will break plugin

I still like the idea of having a grace period with a deprecation warning though. If we just "make it work" now, we are just delaying the problem as it reduces the likelihood of plugin authors realizing that they need to take action

use the beta period as the time for plugins to be updated

That was my original idea actually.

disable them if they are not marked as compatible with 1.3?

I think this may be a good alternative - I think grangeway also suggested something along these lines at some point.

However if we go along this route, we need to notify the admin plugins have been disabled (at upgrade time / during admin checks) to avoid the case of "silent" failures which could lead e.g. to data integrity issues.

I opened child issue 0017522 to track resolution of helper_alternate_class() function; see also PR https://github.com/mantisbt/mantisbt/pull/234

Moved the severity to minor since we seem to be OK with having the beta period as a period for plugin developers to fix their plugins. There is also another issue relating to the most common breaking issue highlighted in this bug.

Helper alternate class issue is now resolved.

Did not look at the other examples you reported, you might want to analyze further and fix that too before we cut the release

The helper_alternate_class() was the main issue. We've decided that we will use the beta stage as a way to get plugins to update themselves. Hence, no further fixes are necessary.

Didn't set fixed in version so as not to show up in change log since the issue was introduced during development for this same release.