Remove deprecated code (Trac 1.5.1)

[Ryan J Ollos]

Like #11901 for 1.3.1, deprecated code scheduled for removal in 1.5.1 will be listed in this ticket.

[Ryan J Ollos]

Is the intention that plugins in 1.5.1 and later should always implement the method signature post_process_request(self, req, template, data, metadata=None), or is def post_process_request(req, template, data) also allowed as a method signature for a class implementing IRequestFilter?

[Ryan J Ollos]

Changes in log:rjollos.git:t12787_remove_deprecated_code. It will be quite a while before the changes are committed, but they could use some review. All tests pass for me, so if there are issues then it will be an opportunity to improve test coverage.

There's one TODO in chrome.py: Chrome.render_template​.

I'll rebase and post revised changes periodically.

[Ryan J Ollos]

Replying to Ryan J Ollos:

Is the intention that plugins in 1.5.1 and later should always implement the method signature post_process_request(self, req, template, data, metadata=None), or is def post_process_request(req, template, data) also allowed as a method signature for a class implementing IRequestFilter?

I think I overlooked that metadata=None was for backward-compatibility, and in the 1.5.1+ API it should be a positional, not a keyword, argument. Therefore the correct signature is post_process_request(self, req, template, data, metadata).

Additional changes in [ff9c6753/rjollos.git]. For the case in which the process_request does not return metadata, an empty dict is passed to the post_process_request methods.

[Ryan J Ollos]

Rebased and additional changes in log:rjollos.git:t12787_remove_deprecated_code.1.

[Ryan J Ollos]

Scheduled EnvironmentStub.clear_component_registry and EnvironmentStub.restore_component_registry for removal in 1.5.1: r15980.

[Ryan J Ollos]

Rebased changes in log:rjollos.git:t12787_remove_deprecated_code.2.

[Ryan J Ollos]

Rebased changes in log:rjollos.git:t12787_remove_deprecated_code.3. Tests pass with and without Genshi.

Is the following needed when support for Genshi is removed?: trunk/trac/util/html.py@16037:712-713#L698​.

Should we allow the metadata argument of process_request to be None with removal of Genshi support? From TracDev/PortingFromGenshiToJinja#IRequestHandler:

Note that as long as we have to support the legacy Genshi templates, a None value passed as third argument won't be interpreted as an empty dict, but rather as an empty content_type.

[Christian Boos]

Replying to Ryan J Ollos:

Rebased changes in log:rjollos.git:t12787_remove_deprecated_code.3. Tests pass with and without Genshi.

Is the following needed when support for Genshi is removed?: trunk/trac/util/html.py@16037:712-713#L698​.

No, I don't think so.

Should we allow the metadata argument of process_request to be None with removal of Genshi support? From TracDev/PortingFromGenshiToJinja#IRequestHandler:

Note that as long as we have to support the legacy Genshi templates, a None value passed as third argument won't be interpreted as an empty dict, but rather as an empty content_type.

The idea was that once we no longer need to play subtle games to differentiate between the two engines, return 'template.html', data should become equivalent to return 'template.html', data, {}.

[Ryan J Ollos]

Replying to Christian Boos:

Is the following needed when support for Genshi is removed?: trunk/trac/util/html.py@16037:712-713#L698​.

No, I don't think so.

I've removed the line and will post the changes after some more time passes and another rebase is needed.

Should we allow the metadata argument of process_request to be None with removal of Genshi support? From TracDev/PortingFromGenshiToJinja#IRequestHandler:

I misspoke here. I was looking at render_template​ and wondering if we should allow metadata to be None. We'd just need to add:

metadata = metadata or {}

[Ryan J Ollos]

Rebased changes in log:rjollos.git:t12787_remove_deprecated_code.4.

[Ryan J Ollos]

Rebased changes in log:rjollos.git:t12787_remove_deprecated_code.5.

[Ryan J Ollos]

Rebased with changes from #13042: log:rjollos.git:t12787_remove_deprecated_code.6.

[Ryan J Ollos]

Rebased again in log:rjollos.git:t12787_remove_deprecated_code.7.

[Ryan J Ollos]

Rebased again in log:rjollos.git:t12787_remove_deprecated_code.8.

[Christian Boos]

That clean-up looks good to me, and simplifies things a lot. However, as trunk seems to be still 1.3dev, I suppose you plan to branch 1.4-stable first, right?

[Ryan J Ollos]

Yeah, I won't put on trunk until trunk is 1.5.1dev. In the meantime I'll continue to rebase the branch to avoid a bigger painful rebase later.

[Ryan J Ollos]

Rebased against r16880 in log:rjollos.git:t12787_remove_deprecated_code.9.

[Christian Boos]

Replying to Ryan J Ollos:

Rebased against r16880 in log:rjollos.git:t12787_remove_deprecated_code.9.

Isn't the add_script(req, 'common/js/excanvas.js') only needed for IE?

[Ryan J Ollos]

Replying to Christian Boos:

Isn't the add_script(req, 'common/js/excanvas.js') only needed for IE?

Yeah, that looks to be the case. I misunderstood the ie_if logic on first read.

[Ryan J Ollos]

comment:19 issue fixed and rebased against r16880 in log:rjollos.git:t12787_remove_deprecated_code.10.

[Ryan J Ollos]

Rebased in log:rjollos.git:t12787_remove_deprecated_code.11.

[Ryan J Ollos]

Rebased in log:rjollos.git:t12787_remove_deprecated_code.12.

[Ryan J Ollos]

Changes committed in [17091-17100].

[anonymous]

#13195:

these lines​ were deleted in r17093.

Can this be reverted for now? It appears to be a regression that breaks built-in functionality.

[Ryan J Ollos]

[17093#file8] removes extract_text. However, this may still be needed for text templates such as batch_ticket_notify_email.txt.

Now, make extraction fails:

$ make extraction
python  setup.py  extract_messages  extract_messages_js  extract_messages_tracini
running extract_messages
extracting messages from tracopt/__init__.py
extracting messages from tracopt/perm/__init__.py
...
extracting messages from trac/ticket/templates/batch_ticket_notify_email.txt (line_comment_prefix="##", line_statement_prefix="#", silent="no", trim_blocks="yes", newstyle_gettext="yes", variable_start_string="${", extensions="jinja2.ext.do, jinja2.ext.with_", variable_end_string="}", lstrip_blocks="yes")
Traceback (most recent call last):
  File "setup.py", line 162, in <module>
    **extra
  File "/Users/rjollos/.pyenv/versions/trac-2.7.15/lib/python2.7/site-packages/setuptools/__init__.py", line 145, in setup
    return distutils.core.setup(**attrs)
  File "/Users/rjollos/.pyenv/versions/2.7.15/lib/python2.7/distutils/core.py", line 151, in setup
    dist.run_commands()
  File "/Users/rjollos/.pyenv/versions/2.7.15/lib/python2.7/distutils/dist.py", line 953, in run_commands
    self.run_command(cmd)
  File "/Users/rjollos/.pyenv/versions/2.7.15/lib/python2.7/distutils/dist.py", line 972, in run_command
    cmd_obj.run()
  File "/Users/rjollos/.pyenv/versions/trac-2.7.15/lib/python2.7/site-packages/babel/messages/frontend.py", line 481, in run
    for filename, lineno, message, comments, context in extracted:
  File "/Users/rjollos/.pyenv/versions/trac-2.7.15/lib/python2.7/site-packages/babel/messages/extract.py", line 157, in extract_from_dir
    dirpath=absname,
  File "/Users/rjollos/.pyenv/versions/trac-2.7.15/lib/python2.7/site-packages/babel/messages/extract.py", line 212, in check_and_call_extract_file
    strip_comment_tags=strip_comment_tags
  File "/Users/rjollos/.pyenv/versions/trac-2.7.15/lib/python2.7/site-packages/babel/messages/extract.py", line 241, in extract_from_file
    strip_comment_tags))
  File "/Users/rjollos/.pyenv/versions/trac-2.7.15/lib/python2.7/site-packages/babel/messages/extract.py", line 294, in extract
    func = getattr(__import__(module, {}, {}, [attrname]), attrname)
AttributeError: 'module' object has no attribute 'extract_text'
make: *** [extraction] Error 1

[Ryan J Ollos]

Replying to Ryan J Ollos:

[17093#file8] removes extract_text. However, this may still be needed for text templates such as batch_ticket_notify_email.txt.

We could alias extract_text to extract_html, which might be better for plugin compatibility:

trac/dist.py

@@ -301 +301 @@
                 else:
                     yield m
 
+    extract_text = extract_html
 
     class generate_messages_js(Command):
         """Generating message javascripts command for use ``setup.py`` scripts.

Or change the extractor for text templates:

messages.cfg

@@ -30 +30 @@
 [extractors]
 python = trac.dist:extract_python
 html = trac.dist:extract_html
-text = trac.dist:extract_text
+text = trac.dist:extract_html

Or make both changes?

[Ryan J Ollos]

Committed comment:32 fix in r17132. New extraction in r17133.

[Ryan J Ollos]

It looks like this code can be removed:

trac/web/__init__.py

@@ -11 +11 @@
 # individuals. For the exact contribution history, see the revision
 # history and logs, available at https://trac.edgewall.org/.
 
-# Workaround for http://bugs.python.org/issue6763 and
-# http://bugs.python.org/issue5853 thread issues
-import mimetypes
-mimetypes.init()
-
 from trac.web.api import *

The issue5853 was fixed in 2009 and Python 2.7 was released in 2010.

[Ryan J Ollos]

comment:34 patch committed in r17167.

[Ryan J Ollos]

Backward-compatibility properties can be removed:

trac/notification/api.py

@@ -325 +325 @@
     distributors = ExtensionPoint(INotificationDistributor)
     subscribers = ExtensionPoint(INotificationSubscriber)
 
-    @property
-    def smtp_always_cc(self):  # For backward compatibility
-        return self.config.get('notification', 'smtp_always_cc')
-
-    @property
-    def smtp_always_bcc(self):  # For backward compatibility
-        return self.config.get('notification', 'smtp_always_bcc')
-
-    @property
-    def ignore_domains(self):  # For backward compatibility
-        return self.config.get('notification', 'ignore_domains')
-
-    @property
-    def admit_domains(self):  # For backward compatibility
-        return self.config.get('notification', 'admit_domains')
-
     @lazy
     def subscriber_defaults(self):
         rawsubscriptions = self.notification_subscriber_section.options()

[Ryan J Ollos]

comment:36 patch applied in r17183.

Revised comments in r17181, r17182.

[Ryan J Ollos]

On trunk trac/templates is added twice to the list of template dirs.

• tags/trac-1.4.1/trac/web/chrome.py@:793#L789​ was uncommented on trunk
• tags/trac-1.4.1/trac/web/chrome.py@:1384#L1374​ should have been removed, but was not

This was an error in r17093.

trac/web/chrome.py

@@ -1307 +1307 @@
                      variable expansion is disabled.
         """
         if not self.jenv:
-            jinja2_dirs = [
-                pkg_resources.resource_filename('trac', 'templates')
-            ] + self.get_all_templates_dirs()
+            jinja2_dirs = self.get_all_templates_dirs()
             self.jenv = jinja2env(
                 loader=FileSystemLoader(jinja2_dirs),
                 auto_reload=self.auto_reload,

[Ryan J Ollos]

comment:38 change committed in r17302.