Problem/Motivation

We're trying to do a little modernization of simpletest/phpunit over in #2641632: Refactor simpletest's *_phpunit_*() (and junit) functions etc. to a class, deprecate

Over there, @dawehner discovered that simpletest_phpunit_configuration_filepath() looks like dead code.

Proposed resolution

Figure out if it's dead code.

Remove it.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#16 interdiff.txt732 bytesmile23
#16 2798273_16.patch916 bytesmile23
#13 interdiff-2798273.txt600 bytespfructuoso
#13 2798273-13.patch911 bytespfructuoso
#10 2798273_10.patch773 bytesmile23
#2 2798273_2.patch669 bytesmile23

Comments

Mile23 created an issue. See original summary.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Active » Needs review
StatusFileSize
new 2798273_2.patch669 bytes
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Reviewed & tested by the community

Had a grep around and it is indeed dead, thanks @Mile23

dawehner’s picture

dawehner
Primary language German
commented

Well, people could still see this as some sort of BC break.

catch’s picture

catch
he/him
Primary language English
commented

Yes postponing on the current discussion happening at #2550249: [meta] Document @internal APIs both explicitly in phpdoc and implicitly in d.o documentation about procedural functions, since we don't have an explicit policy for them at the moment.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Postponed

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Parent issue: » #2866082: [Plan] Roadmap for Simpletest
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Title: simpletest_phpunit_configuration_filepath() is dead code » simpletest_phpunit_configuration_filepath() is dead code. Deprecate it.
Status: Postponed » Needs work

So should we mark the function as deprecated?

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review
Issue tags: +@deprecated
StatusFileSize
new 2798273_10.patch773 bytes

This patch just deprecates the function.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Reviewed & tested by the community

Yes:)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/modules/simpletest/simpletest.module
@@ -285,6 +285,12 @@ function simpletest_phpunit_xml_filepath($test_id) {
+ * @deprecated in Drupal 8.4.x for removal before Drupal 9.0.0. PHPUnit test

Shouldn't we have an accompanying trigger_error(…, E_USER_DEPRECATED) call?

pfructuoso’s picture

pfructuoso commented
StatusFileSize
new 2798273-13.patch911 bytes
new interdiff-2798273.txt600 bytes

@Wim Leers, good point. Lets see the deprecation howto

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community
cilefen’s picture

cilefen commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/simpletest/simpletest.module
@@ -285,8 +285,15 @@ function simpletest_phpunit_xml_filepath($test_id) {
+ * @see \simpletest_phpunit_run_command()

I cannot find an example in core where we namespace a @see to a global function.

An @deprecated PHPdoc tag that indicates when the code was deprecated, when it will be removed, and what to use instead, usually with a link to the change record for the change. — https://www.drupal.org/core/deprecation

A change record is not necessary. Does anyone think this deprecation warrants one?

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review
StatusFileSize
new 2798273_16.patch916 bytes
new interdiff.txt732 bytes

If there were usages of this function then we'd for sure need to make a change record. I'm pretty sure we don't need one.

Here are the standards for @see: https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...

Global functions should not be namespaced but we don't have a coder rule for it yet, apparently.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Cool, one less thing to care about.

  • cilefen committed dccab71 on 8.4.x 
    Issue #2798273 by Mile23, tar_inet:...
cilefen’s picture

cilefen commented
Status: Reviewed & tested by the community » Fixed

Committed dccab71 and pushed to 8.4.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.