Edgewall Software
Modify

Opened 7 years ago

Closed 5 years ago

Last modified 4 years ago

#12787 closed task (fixed)

Remove deprecated code (Trac 1.5.1)

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.5.1
Component: general Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:
API Changes:

Removed deprecated code:

  • Genshi template framework removed
  • ITemplateStreamFilter removed
  • IRequestFilter.post_process_request method argument removed and metadata argument is required rather than optional
  • InvalidAttachment and InvalidTicket exceptions removed
  • Accessors removed in TicketModule for moved options: max_comment_size, max_description_size, max_summary_size (moved to TicketSystem)
  • Environment.get_systeminfo removed
  • EnvironmentStub.clear_component_registry and EnvironmentStub.restore_component_registry removed
  • IAttachmentChangeListener.attachment_reparented and Attachment.reparent removed
  • Deprecated parameters of Query methods removed
  • Request.send_error no longer renders an error page
  • charset and ie_if attributes from script elements removed (and from function add_script)
  • excanvas.js removed
  • NotificationSystem (trac.notification.api) properties removed: smtp_always_cc, smtp_always_bcc, ignore_domains, admit_domains
Internal Changes:

Description

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

Attachments (0)

Change History (39)

comment:1 by Ryan J Ollos, 7 years ago

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?

comment:2 by Ryan J Ollos, 7 years ago

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.

Version 0, edited 7 years ago by Ryan J Ollos (next)

in reply to:  1 comment:3 by Ryan J Ollos, 7 years ago

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.

comment:4 by Ryan J Ollos, 7 years ago

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

comment:5 by Ryan J Ollos, 7 years ago

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

comment:6 by Ryan J Ollos, 7 years ago

Milestone: next-major-releases1.5.1

comment:8 by Ryan J Ollos, 7 years ago

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.

in reply to:  8 ; comment:9 by Christian Boos, 7 years ago

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, {}.

in reply to:  9 comment:10 by Ryan J Ollos, 7 years ago

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 {}

comment:13 by Ryan J Ollos, 6 years ago

comment:16 by Christian Boos, 5 years ago

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?

comment:17 by Ryan J Ollos, 5 years ago

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.

comment:18 by Ryan J Ollos, 5 years ago

in reply to:  18 ; comment:19 by Christian Boos, 5 years ago

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?

Last edited 5 years ago by Christian Boos (previous) (diff)

in reply to:  19 comment:20 by Ryan J Ollos, 5 years ago

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.

comment:21 by Ryan J Ollos, 5 years ago

comment:22 by Ryan J Ollos, 5 years ago

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

comment:24 by Ryan J Ollos, 5 years ago

Owner: set to Ryan J Ollos
Status: newassigned

comment:25 by Ryan J Ollos, 5 years ago

Type: defecttask

comment:26 by Ryan J Ollos, 5 years ago

API Changes: modified (diff)
Resolution: fixed
Status: assignedclosed

Changes committed in [17091-17100].

comment:27 by anonymous, 5 years ago

API Changes: modified (diff)

comment:28 by anonymous, 5 years ago

API Changes: modified (diff)

comment:29 by Ryan J Ollos, 5 years ago

API Changes: modified (diff)

comment:30 by anonymous, 5 years ago

#13195:

these lines were deleted in r17093.

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

comment:31 by Ryan J Ollos, 5 years ago

[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

in reply to:  31 comment:32 by Ryan J Ollos, 5 years ago

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

    diff --git a/trac/dist.py b/trac/dist.py
    index 5b9ca9815..9f5f6584a 100644
    a b try:  
    301301                else:
    302302                    yield m
    303303
     304    extract_text = extract_html
    304305
    305306    class generate_messages_js(Command):
    306307        """Generating message javascripts command for use ``setup.py`` scripts.

Or change the extractor for text templates:

  • messages.cfg

    diff --git a/messages.cfg b/messages.cfg
    index 0ae9d5b47..10a68fe85 100644
    a b silent = no  
    3030[extractors]
    3131python = trac.dist:extract_python
    3232html = trac.dist:extract_html
    33 text = trac.dist:extract_text
     33text = trac.dist:extract_html

Or make both changes?

comment:33 by Ryan J Ollos, 5 years ago

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

comment:34 by Ryan J Ollos, 5 years ago

It looks like this code can be removed:

  • trac/web/__init__.py

    diff --git a/trac/web/__init__.py b/trac/web/__init__.py
    index 4769035f3..b71680001 100644
    a b  
    1111# individuals. For the exact contribution history, see the revision
    1212# history and logs, available at https://trac.edgewall.org/.
    1313
    14 # Workaround for http://bugs.python.org/issue6763 and
    15 # http://bugs.python.org/issue5853 thread issues
    16 import mimetypes
    17 mimetypes.init()
    18 
    1914from trac.web.api import *

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

comment:35 by Ryan J Ollos, 5 years ago

comment:34 patch committed in r17167.

comment:36 by Ryan J Ollos, 5 years ago

Backward-compatibility properties can be removed:

  • trac/notification/api.py

    diff --git a/trac/notification/api.py b/trac/notification/api.py
    index 9f85efa98..b3066705d 100644
    a b class NotificationSystem(Component):  
    325325    distributors = ExtensionPoint(INotificationDistributor)
    326326    subscribers = ExtensionPoint(INotificationSubscriber)
    327327
    328     @property
    329     def smtp_always_cc(self):  # For backward compatibility
    330         return self.config.get('notification', 'smtp_always_cc')
    331 
    332     @property
    333     def smtp_always_bcc(self):  # For backward compatibility
    334         return self.config.get('notification', 'smtp_always_bcc')
    335 
    336     @property
    337     def ignore_domains(self):  # For backward compatibility
    338         return self.config.get('notification', 'ignore_domains')
    339 
    340     @property
    341     def admit_domains(self):  # For backward compatibility
    342         return self.config.get('notification', 'admit_domains')
    343 
    344328    @lazy
    345329    def subscriber_defaults(self):
    346330        rawsubscriptions = self.notification_subscriber_section.options()

comment:37 by Ryan J Ollos, 5 years ago

API Changes: modified (diff)

comment:36 patch applied in r17183.

Revised comments in r17181, r17182.

comment:38 by Ryan J Ollos, 4 years ago

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

This was an error in r17093.

  • trac/web/chrome.py

    diff --git a/trac/web/chrome.py b/trac/web/chrome.py
    index cb36454d5..2f663e13a 100644
    a b class Chrome(Component):  
    13071307                     variable expansion is disabled.
    13081308        """
    13091309        if not self.jenv:
    1310             jinja2_dirs = [
    1311                 pkg_resources.resource_filename('trac', 'templates')
    1312             ] + self.get_all_templates_dirs()
     1310            jinja2_dirs = self.get_all_templates_dirs()
    13131311            self.jenv = jinja2env(
    13141312                loader=FileSystemLoader(jinja2_dirs),
    13151313                auto_reload=self.auto_reload,
Last edited 4 years ago by Ryan J Ollos (previous) (diff)

comment:39 by Ryan J Ollos, 4 years ago

comment:38 change committed in r17302.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.