Remove deprecated db parameters

[Ryan J Ollos]

Remove deprecated db parameters which were scheduled for removal in Trac 1.1.1, such as trunk/trac/ticket/model.py@12751:86,88-89#L73​.

[Ryan J Ollos]

ReportModule.execute_paginated_report​ has a db positional parameter. We would have to change the method signature to fix the issue, however there are no cases of this method being used on trac-hacks:

$ grep -r ".\execute_paginated_report" trachacks.git/
$

[Ryan J Ollos]

I guess we should keep the db parameters in IEnvironmentUpgradeParticipant methods, but maybe the parameters should be marked as deprecated: tags/trac-1.0.1/trac/env.py@:76,84#L62​.

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t11605. The changes address the items raised in comment:1 and comment:2.

[Jun Omae]

I don't think it is good for [a9d85e3a/rjollos.git].

I think that no need to remove db parameter from private methods and those methods seems to be private but those are not private for derived classes. Also, the changes lead unnecessary nested transaction context, with_transaction.

[Jun Omae]

.. those are not private for derived classes.

The _insert_changeset method is used in [0068e373/jomae.git] from jomae.git@t10602.1 which have not been pushed yet (#10602).

[Ryan J Ollos]

Replying to jomae:

I think that no need to remove db parameter from private methods and those methods seems to be private but those are not private for derived classes.

That's not a good reason to avoid pushing those changes. A private method is exactly that, and your changes would just need to be rebased to adapt to the internal changes.

Also, the changes lead unnecessary nested transaction context, with_transaction.

That may be a good reason to not push the change.

[Jun Omae]

CachedRepository.sync() isn't good to git repository with cache, e.g. tracopt.versioncontrol.git (#10602), ​th:TracPygit2Plugin and ​my GitPlugin. I think we need overwrite sync method of CachedGitRepository in tracopt.versioncontrol.git and the plugins and I'll do it using CachedRepository._insert_changeset in Trac core and the plugins.

If signature of the method is changed, the change is breaking backward-compatibilities and differences signatures between Trac versions will be unfortunate to me.

Also, I think changes for ReportModule.execute_paginated_report​ is breaking backward-compatibilities. Thing which method is currently not used by the plugins cannot be good reason of those changes which breaks backward-compatibilities. If anyone create cross-version plugins in the future, the changes will annoy the authors.

[Ryan J Ollos]

If we need to be concerned about changing the signature of _insert_changeset then it should really be a public method. The whole point of making a method private is that it shouldn't be accessed by external components. So the plugins are wrong for accessing that method and expecting a contract to be adhered to in doing so. Practicality prevails though, so I agree that we shouldn't change the method. We should figure out an API change that could be made so that those plugins don't need to access a private method of the class.

As for ReportModule.execute_paginated_report​, we are making the changes on the trunk so breaking compatibilities is allowed. We shouldn't intentionally cause anyone pain, but if the method is unlikely to be used by anyone and we can't see it being used by anyone, then I would prefer to cleanup the API. I think you are being overly pedantic.

I will push [c1d04785/rjollos.git], [766aa7c8/rjollos.git] and [af772265/rjollos.git] and close out this ticket.

[Ryan J Ollos]

I think we should give [2fb1f6e1/rjollos.git] further consideration. By not changing the method, we are leaving an inconsistency in the API. There are no other public method which get passed a db object, and so the inconsistency will be strange for users of the API. As far as I can see there is no way to ever change this method without breaking backward compatibility. Making the change for Trac 1.2 is just as good as making the change in Trac 1.4.

Any third opinions on the matter?

[Jun Omae]

If we need to be concerned about changing the signature of _insert_changeset then it should really be a public method. The whole point of making a method private is that it shouldn't be accessed by external components. So the plugins are wrong for accessing that method and expecting a contract to be adhered to in doing so. Practicality prevails though, so I agree that we shouldn't change the method. We should figure out an API change that could be made so that those plugins don't need to access a private method of the class.

Thanks. I think We could add insert_changeset without db parameter and mark _insert_changeset as "deprecated" like the following in this ticket. If the changes is applied, we keep compatibilities and external plugins can easily detect the method with/without db parameter.

trac/versioncontrol/cache.py

@@ -231 +231 @@
                     cset = self.repos.get_changeset(next_youngest)
                     try:
                         # steps 1. and 2.
-                        self._insert_changeset(db, next_youngest, cset)
+                        self.insert_changeset(next_youngest, cset)
                     except Exception as e: # *another* 1.1. resync attempt won
                         self.log.warning('Revision %s already cached: %r',
                                          next_youngest, e)
@@ -259 +259 @@
                 if feedback:
                     feedback(youngest)
 
+    def insert_changeset(self, rev, cset):
+        """create revision and node_change records for the given changeset
+        instance.
+        """
+        with self.env.db_transaction as db:
+            self._insert_changeset(db, rev, cset)
+
     def _insert_changeset(self, db, rev, cset):
+        """:deprecated: since 1.1.2, use `insert_changeset` instead and will
+        be removed in 1.4.
+        """
         srev = self.db_rev(rev)
         # 1. Attempt to resync the 'revision' table.  In case of
         # concurrent syncs, only such insert into the `revision` table

As for ReportModule.execute_paginated_report​, we are making the changes on the trunk so breaking compatibilities is allowed. We shouldn't intentionally cause anyone pain, but if the method is unlikely to be used by anyone and we can't see it being used by anyone, then I would prefer to cleanup the API. I think you are being overly pedantic.

Yes, I think I'm so pedantic about this. However, Trac developers have kept backward-compatibilities of ReportModule.execute_report​ in [7214] but the method is unused in trac-hacks, ​https://github.com/rjollos/trachacks/search?q=execute_report.

[Ryan J Ollos]

I've refreshed the changes to include the feedback from comment:11: log:rjollos.git:t11605.2. There may be a way to change execute_paginated_report and keep it backwards compatible: [31283efc/rjollos.git]. An additional unit test would be needed to verify proper behavior and the docstring would be improved.

[Jun Omae]

After [12826], unit tests fail.

======================================================================
ERROR: test_quoted_id_with_var (trac.ticket.tests.report.ReportTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/run/shm/d5faeed17395bdbbc1f891ac2b80e29c5a06b7f2/py26-sqlite/trac/ticket/tests/report.py", line 127, in test_quoted_id_with_var
    {'USER': 'joe'})
  File "/run/shm/d5faeed17395bdbbc1f891ac2b80e29c5a06b7f2/py26-sqlite/trac/ticket/report.py", line 654, in execute_paginated_report
    sql, args, missing_args = self.sql_sub_vars(sql, args, db)
TypeError: sql_sub_vars() takes exactly 3 arguments (4 given)

...
----------------------------------------------------------------------
Ran 1602 tests in 45.879s

FAILED (errors=9)
make: *** [unit-test] Error 1
make: Leaving directory `/run/shm/d5faeed17395bdbbc1f891ac2b80e29c5a06b7f2/py26-sqlite'

[Ryan J Ollos]

Committed to trunk in [12826:12831]. I didn't take enough care for testing intermediate revisions to be sure that the tests didn't fail.

[Ryan J Ollos]

Here is a case I missed due to grepping for just db: browser:/trunk/trac/env.py@12829:676,684-688#L675​.

[Ryan J Ollos]

#11639 reported an issue with ​th:TimingAndEstimationPlugin that is due to removal of the deprecated parameters.

[Ryan J Ollos]

Change from comment:15 committed to trunk in [12842].

[Ryan J Ollos]

Another deprecated parameter removed in [12854].

[Ryan J Ollos]

In [12856:12857], removed support for db parameter from methods decorated with cached.

[Ryan J Ollos]

Eventually we may want to remove Environment.get_db_cnx, Environment.get_read_db and Environment.with_transaction:

• trunk/trac/env.py@12843:430#L419​
• trunk/trac/env.py@12843:473,477#L456​

Shall we add to the docstring that the methods will be removed in 1.3.1? Similar changes are scheduled for 1.3.1 already:

• trunk/trac/env.py@12843:82-84#L61​
• trunk/trac/env.py@12843:99-101#L87​

[Peter Suter]

Replying to rjollos:

Shall we add to the docstring that the methods will be removed in 1.3.1?

Sounds like a good idea to me.

[Ryan J Ollos]

Thanks for the feedback. Committed in [12970], merged in [12971].

[Ryan J Ollos]

Replying to rjollos:

Eventually we may want to remove Environment.get_db_cnx, Environment.get_read_db and Environment.with_transaction:

Environment.get_db_cnx had already been scheduled for removal in 1.1.1: TracDev/ApiChanges/1.1.2.

I removed it in [13064] and fixed the comment from [12970] in [13065] (followed by record-only merge of that changeset in [13066]).

[Ryan J Ollos]

According to TracDev/ApiChanges#GuidelinesforAPIchanges, we should log a deprecation warning when calling deprecated functions and methods. We could do it in the following places:

• trunk/trac/ticket/report.py@12834:664#L649​ - Just above the highlighted line.
• trunk/trac/versioncontrol/cache.py@13020:275#L269​ - We'd need to extract the body of the _insert_changeset method to a third method, or somehow determine the caller is not insert_changeset.
• trunk/trac/env.py@13064:717#L712​ - Add an else.
• trunk/trac/env.py@13064:754#L725​ - Add an else.

The changes could probably wait until milestone:1.1.3.

[Ryan J Ollos]

With all of the API changes being made in 1.3.1, we could take the opportunity to also remove the authname parameter from Environment.get_repository or even remove the get_repository method altogether since it is stated that it has only been left for backward-compatibility: trunk/trac/env.py@13064:551-564#L541​. Any suggestions on which direction to go?

[Ryan J Ollos]

TracInstall#OtherPythonPackages states that SilverCity and Enscript support is deprecated. Shall we remove these in milestone:1.1.3?

The SilverCity page claims support was dropped in Trac 0.13. Enscript and PHP renderers were deprecated in #8113.

[Ryan J Ollos]

We could also remove support for mod_python (TracModPython) in milestone:1.1.3. I still see users on running mod_python with Trac 1.0, and it would be good to push them toward mod_wsgi so that we aren't handling bug reports many years from now from users still running mod_python. Thoughts?

[Jun Omae]

Replying to rjollos:

With all of the API changes being made in 1.3.1, we could take the opportunity to also remove the authname parameter from Environment.get_repository or even remove the get_repository method altogether since it is stated that it has only been left for backward-compatibility: trunk/trac/env.py@13064:551-564#L541​. Any suggestions on which direction to go?

I agree the removing authname parameter. That parameter is unused since 0.12.

[Jun Omae]

Replying to rjollos:

TracInstall#OtherPythonPackages states that SilverCity and Enscript support is deprecated. Shall we remove these in milestone:1.1.3?

The SilverCity page claims support was dropped in Trac 0.13. Enscript and PHP renderers were deprecated in #8113.

It looks okay to me. That statement for SilverCity and Enscript support has been added to wiki:0.12/TracInstall@8?action=diff before 0.12 is released.

[Jun Omae]

Replying to rjollos:

We could also remove support for mod_python (TracModPython) in milestone:1.1.3. I still see users on running mod_python with Trac 1.0, and it would be good to push them toward mod_wsgi so that we aren't handling bug reports many years from now from users still running mod_python. Thoughts?

Seems that ​mod_python project is alive. Recently, 3.5.0 is released on Nov-13-2013.

[Ryan J Ollos]

Replying to jomae:

Seems that ​mod_python project is alive. Recently, 3.5.0 is released on Nov-13-2013.

I guess we should wait and see on that one. I was curious if anyone has run version 3.5.0 with Trac, so I asked on the mailing list: ​trac-users:1VzLgLT4gDE.

[Peter Suter]

Replying to rjollos:

Committed to trunk in [12826:12831].

After this, running python -m trac.test fails at the functional tests because somehow LegacyParticipant is still enabled(?) from the unit tests.

[Ryan J Ollos]

Replying to jomae:

Replying to rjollos:

TracInstall#OtherPythonPackages states that SilverCity and Enscript support is deprecated. Shall we remove these in milestone:1.1.3?

The SilverCity page claims support was dropped in Trac 0.13. Enscript and PHP renderers were deprecated in #8113.

It looks okay to me. That statement for SilverCity and Enscript support has been added to wiki:0.12/TracInstall@8?action=diff before 0.12 is released.

Dedicated ticket is now #11795.

[Ryan J Ollos]

Replying to jomae:

Replying to rjollos:

With all of the API changes being made in 1.3.1, we could take the opportunity to also remove the authname parameter from Environment.get_repository or even remove the get_repository method altogether since it is stated that it has only been left for backward-compatibility: trunk/trac/env.py@13064:551-564#L541​. Any suggestions on which direction to go?

I agree the removing authname parameter. That parameter is unused since 0.12.

Removed in [13193], see comment:12:ticket:11703.

[Ryan J Ollos]

Replying to psuter:

Replying to rjollos:

Committed to trunk in [12826:12831].

After this, running python -m trac.test fails at the functional tests because somehow LegacyParticipant is still enabled(?) from the unit tests.

Yeah I can reproduce. Good catch. I hadn't seen the issue running make test (or equivalently python ./trac/test.py --skip-functional-tests followed by python trac/tests/functional/__init__.py -v).

The issue goes away when the two methods are commented out: trunk/trac/tests/env.py@:134,156#L133​, but I'm not sure why the LegacyParticipants are enabled for the functional tests.

[Peter Suter]

Replying to rjollos:

but I'm not sure why the LegacyParticipants are enabled for the functional tests.

By default all components in the trac packages are enabled, except trac.test.*​ (since #11421) but trac.tests.env.LegacyParticipant doesn't match that (test vs. tests).

[Peter Suter]

Replying to psuter:

After this, running python -m trac.test fails at the functional tests because somehow LegacyParticipant is still enabled(?) from the unit tests.

I created #11815.

[Ryan J Ollos]

Replying to rjollos:

In [12856:12857], removed support for db parameter from methods decorated with cached.

In [13461], removed import that should have been removed in [12856].

[Ryan J Ollos]

Replying to rjollos:

Change from comment:15 committed to trunk in [12842].

Change to EnvironmentStub.get_known_users in [13465].

[Ryan J Ollos]

Replying to rjollos:

Replying to jomae:

Seems that ​mod_python project is alive. Recently, 3.5.0 is released on Nov-13-2013.

I guess we should wait and see on that one. I was curious if anyone has run version 3.5.0 with Trac, so I asked on the mailing list: ​trac-users:1VzLgLT4gDE.

The deprecation warning was removed by a wiki editor in TracModPython@165. I guess we will continue to support it unless someone has additional input.