Problem/Motivation

Out there in #2950132: Support PHPUnit 7 optionally in Drupal 8, while keeping support for ^6.5, some FileFieldTestBase tests fail.

That is due to the following code:

  /**
   * Asserts that a file exists physically on disk.
   *
   * Overrides PHPUnit\Framework\Assert::assertFileExists() to also work with
   * file entities.
   *
   * @param \Drupal\File\FileInterface|string $file
   *   Either the file entity or the file URI.
   * @param string $message
   *   (optional) A message to display with the assertion.
   */
  public static function assertFileExists($file, $message = NULL) {
    $message = isset($message) ? $message : format_string('File %file exists on the disk.', ['%file' => $file->getFileUri()]);
    $filename = $file instanceof FileInterface ? $file->getFileUri() : $file;
    parent::assertFileExists($filename, $message);
  }

....

  /**
   * Asserts that a file does not exist on disk.
   *
   * Overrides PHPUnit\Framework\Assert::assertFileExists() to also work with
   * file entities.
   *
   * @param \Drupal\File\FileInterface|string $file
   *   Either the file entity or the file URI.
   * @param string $message
   *   (optional) A message to display with the assertion.
   */
  public static function assertFileNotExists($file, $message = NULL) {
    $message = isset($message) ? $message : format_string('File %file exists on the disk.', ['%file' => $file->getFileUri()]);
    $filename = $file instanceof FileInterface ? $file->getFileUri() : $file;
    parent::assertFileNotExists($filename, $message);
  }

these methods that override PHPUnit's ones raise fatal errors because in PHPUnit7 they are signed with scalar type hints, so you can no longer overload $file being either a string or a FileInterface variable.

Proposed resolution

Explicitly pass the File entity URI via File::getFileUri() to assertFileExists and assertFileNotExists, and conditionally deprecate the two current methods that override PHPUnit ones, when FileInterface arguments are passed in.

Remaining tasks

User interface changes

none

API changes

none

Data model changes

none

Release notes snippet

CommentFileSizeAuthor
#25 3031539-25.patch18.84 KBmondrake
#25 interdiff_23-25.txt897 bytesmondrake
#23 interdiff_14-23.txt19.59 KBmondrake
#23 3031539-23.patch18.84 KBmondrake
#14 3031539-14.patch19.74 KBmondrake
#14 interdiff_12-14.txt646 bytesmondrake
#12 3031539-12.patch19.75 KBmondrake
#12 interdiff_10-12.txt2.71 KBmondrake
#10 interdiff_2-10.txt4.61 KBmondrake
#10 3031539-10.patch17.03 KBmondrake
#2 3031539-2.patch17.28 KBmondrake

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
commented
StatusFileSize
new 3031539-2.patch17.28 KB

Here's a patch. We'll have to deprecate in 8.7.x and remove in 8.8.x if we want to have a chance to have PHPUnit 7 test runs with 8.8.x - otherwise test will fail because assertFileExists and assertFileNotExists, being direct overrides of PHPUnit methods, will fail because of changed method signature.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Needs review
mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

amateescu’s picture

amateescu commented
Status: Needs review » Needs work

This patch looks great to me! Just two things to fix clarify:

  1. +++ b/core/modules/file/tests/src/Functional/FileFieldTestBase.php
@@ -202,11 +202,35 @@ public function replaceNodeFile($file, $field_name, $nid, $new_revision = TRUE)
+   * @deprecated as of Drupal 8.7.x, will be removed in Drupal 8.8.0. Use

    Since Drupal 8.7 has been released, I think we need to move the deprecation message to 8.8.x. Also, it should be removed in Drupal 9, not in 8.9.x.

    Also, this deprecated tag marks the method as deprecated (in IDEs) even when it's used with a string argument, which is not what we want. Tricky situation... 

  2. +++ b/core/modules/file/tests/src/Functional/FileFieldTestBase.php
@@ -202,11 +202,35 @@ public function replaceNodeFile($file, $field_name, $nid, $new_revision = TRUE)
+   * @see https://www.drupal.org/node/xxx

    We need a change record for this :)

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue tags: +Novice

#6.1 actually I think we should not add the @deprecated annotation at all - these are valid PHPUnit methods :) Just keep the conditional @trigger_error.

amateescu’s picture

amateescu commented

Yup, I think that's the best way forward. The CR should explicitly say that only passing a file entity as the first argument to assertFileExists and assertFileNotExists is deprecated.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Filed draft CR https://www.drupal.org/node/3057326

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
StatusFileSize
new 3031539-10.patch17.03 KB
new interdiff_2-10.txt4.61 KB

Addressing #6 and #7.

Status: Needs review » Needs work

The last submitted patch, 10: 3031539-10.patch, failed testing. View results

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
Issue tags: -Novice
StatusFileSize
new interdiff_10-12.txt2.71 KB
new 3031539-12.patch19.75 KB

#10 good, deprecation tests work :)

Status: Needs review » Needs work

The last submitted patch, 12: 3031539-12.patch, failed testing. View results

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff_12-14.txt646 bytes
new 3031539-14.patch19.74 KB

Typo :(

amateescu’s picture

amateescu commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +PHPUnit

Thanks for the quick updates, the patch looks great to me!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 3031539-14.patch, failed testing. View results

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Reviewed & tested by the community
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented 
  1. +++ b/core/modules/file/tests/src/Functional/FileFieldTestBase.php
@@ -209,11 +209,31 @@ public function replaceNodeFile($file, $field_name, $nid, $new_revision = TRUE)
   public static function assertFileExists($file, $message = NULL) {
...
+  public static function assertManagedFileExists(FileInterface $file, $message = NULL) {

    any reason these are static? 

  2. +++ b/core/modules/file/tests/src/Functional/FileFieldTestBase.php
@@ -209,11 +209,31 @@ public function replaceNodeFile($file, $field_name, $nid, $new_revision = TRUE)
+    else {

@@ -229,18 +249,38 @@ public function assertFileEntryExists($file, $message = NULL) {
+    else {

    we could return early and avoid the else here

Playing devils advocate, if we implemented __toString on the file entity, would this issue go away?

mondrake’s picture

mondrake
Location 🇮🇹
commented
Assigned: Unassigned » mondrake
Status: Reviewed & tested by the community » Needs work

On it.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

> any reason these are static?

Probably consistency with phpunit assert methods, which are usually static?

> Playing devils advocate, if we implemented __toString on the file entity, would this issue go away?

I don't think we should do that. These methods have nothing to to with their parents, I assume they were added long before we converted the test to phpunit, I prefer renaming them. Adding __toString() (that would do what, return the ID?) to file entities seems pretty arbitrary.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

that would do what, return the ID?

return the uri, which is the change we make in assertManagedFile

happy for the consensus to be that's a dumb idea

mondrake’s picture

mondrake
Location 🇮🇹
commented

1. Yes, was following PHPUnit pattern where most assert* methods are static. In fact there's even an issue #3029358: Change tests calling static methods as '$this->' to 'static::' to convert assert calls to static::

2.

if we implemented __toString on the file entity

I'm 99% sure it wont work anyway, because in PHPUnit 7 the assertFileExists method is function assertFileExists(string $filename, string $message = ''): void, and I think type checking is strict, so passing the entity (to be casted to string) will fail. But the @larowlan's hint is good: we already have a File::getFileUri method, so how about just dropping assertManagedFileExists and assertManagedFileNotExists (less API), and just call that method in tests like assertFileExists($file->getFileUri) (more readable tests)?

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
StatusFileSize
new 3031539-23.patch18.84 KB
new interdiff_14-23.txt19.59 KB

Patch, doing #22.2

Status: Needs review » Needs work

The last submitted patch, 23: 3031539-23.patch, failed testing. View results

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff_23-25.txt897 bytes
new 3031539-25.patch18.84 KB

Ouch.

amateescu’s picture

amateescu commented
Status: Needs review » Reviewed & tested by the community

I like the new direction a lot, "less API"++ :)

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes
mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes

Updated IS and draft CR.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

So in Drupal 9 we can remove these method overrides. Nice work - great solution. It does mean we might be stuck on PHPUnit6 for Drupal 8 but this makes crossing that bridge if we have to a little easier.

Committed 3954b06 and pushed to 8.8.x. Thanks!

  • alexpott committed 3954b06 on 8.8.x 
    Issue #3031539 by mondrake, amateescu, larowlan, Berdir:...
mondrake’s picture

mondrake
Location 🇮🇹
commented

Published CR.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

> It does mean we might be stuck on PHPUnit6 for Drupal 8 but this makes crossing that bridge if we have to a little easier.

Yeah, I think if this ends up being the only reason we don't update in D8 then we should just drop this method. I didn't find any usages of this on http://grep.xnddx.ru, there are a few subclasses, but afaik none that uses this helper method.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Actually, if we were to introduce PHPUnit-version-dependent traits like I am suggesting in #2950132: Support PHPUnit 7 optionally in Drupal 8, while keeping support for ^6.5, we can test on PHPUnit 6 with the overriden method and keep the deprecation, or test on PUHPunit 7 with the 'native' method and fail if an object is passed to assertFileExists. What we cannot have is a deprecation on PHPUnit 7, because of the strict typing of the method itself. See #2950132-89: Support PHPUnit 7 optionally in Drupal 8, while keeping support for ^6.5.

Status: Fixed » Closed (fixed)

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

mondrake’s picture

mondrake
Location 🇮🇹
commented
Assigned: mondrake » Unassigned