🇮🇳India @mohit_aghera

mohit_aghera → made their first commit to this issue’s fork.

@mohit_agher 🐛 Fix null string error passed to realpath() in LocalStream Closed: cannot reproduce

I came across this issue as part of Bugsmash triage.

This seems already fixed as part of #3101552
Code is pretty much same.

We should close this issue.

I came across this issue while doing Bugsmash triage.
Thank you for reporting the issue.

I am not able to reproduce the issue since there are no clear steps to reproduce.
Considering it is not easily reproducible, I'm closing the issue.

Please reopen again if you feel issue is happening.

mohit_aghera → created an issue.

I've visited this issue as part of bug-smash triage. Sharing the finding.

The following config seems invalid to me.

title:
          id: title
          table: node_field_data
          field: title
          type: file_link
          settings:
            link_to_entity: false

Reason is, field formatter `file_link` doesn't have any setting called `link_to_entity`.
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/file/...
Even it's base class doesn't have `link_to_entity` either.

To debug further why this option appears, I checked FormatterPluginManager class.
This class instantiates the formatter.
So when we are adding `file_link` formatter on title, the control comes in getInstance() method of the class.
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

In the getInstance() method, incoming argument array `$options` is correct and it contains the `file_link` formatter.
i.e. $options['configuration']['type'] value is `file_link`.

Now in the following snippet is swapping `file_link` formatter with the string formatter.

if (!isset($definition['class']) || !in_array($field_type, $definition['field_types']) || !$definition['class']::isApplicable($field_definition)) {
      // Grab the default widget for the field type.
      $field_type_definition = $this->fieldTypeManager->getDefinition($field_type);
      if (empty($field_type_definition['default_formatter'])) {
        return FALSE;
      }
      $plugin_id = $field_type_definition['default_formatter'];
    }

The following condition is failing.
$definition['class']::isApplicable($field_definition)
Because BaseFieldFileFormatterBase class' isApplicable() fails since $field_definition for title field doesn't have target file type.
Since that fails, we are swapping the formatter with string formatter and it displays the `link_to_entity` form and it has configuration.

Possibly one solution can be to restrict the options in field formatter options when we configure the field in views.
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/views...

We can filter options in `buildOptionsForm()`

  public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    parent::buildOptionsForm($form, $form_state);

    $field = $this->getFieldDefinition();
    $formatters = $this->formatterPluginManager->getOptions($field->getType());
    foreach ($formatters as $id => $formatter) {
      $definition = $this->formatterPluginManager->getDefinition($id);
      if (!$definition['class']::isApplicable($field)) {
        unset($formatters[$id]);
      }
    }

I'm not quite sure if this is the right approach.

This is probably the duplicate of #3090998
The other issue has tests as well.

I feel we should close this issue and direct efforts to the other issue.

@herved @Omega_yang
Can you please try the patch in the #3090998 and see if it solves the issues.

Closing the issue for now.
Please re-open again if you feel the mentioned issue doesn't solve the issue.
If you see anything missing, I can update the patch in the other issues to make it work.

This issue doesn't exist anymore.
File upload handler has updated the message and the current updated message contains actual filename.

https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/file/...

   $filename = $event->getFilename();

    $mimeType = $this->mimeTypeGuesser->guessMimeType($filename);
    $destinationFilename = $this->fileSystem->getDestinationFilename($destination . $filename, $fileExists);
    if ($destinationFilename === FALSE) {
      throw new FileExistsException(sprintf('Destination file "%s" exists', $destinationFilename));
    }

I think we should close this.
Please reopen if you feel it is still valid.

I've debug the issue further.
Duplicate form is appearing because we are returning $form in the field formatter.
When call comes to field formatter, it already has all the options in the form. See https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/views...

Once we return $form as it is, it gets merged into existing form array.
Line 540-542 https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/views...

This is causing the duplicate form fields.

I believe returning empty array should fix the problem.

Moving back to RTBC since the MR is already green.

True that.
However, if I don't add those, phpstan is failing for our test case class.
Job https://git.drupalcode.org/issue/drupal-3090998/-/jobs/5656011 has more information.

mohit_aghera → changed the visibility of the branch 3446848-file-fields-that to hidden.

mohit_aghera → made their first commit to this issue’s fork.

Ohh, yeah!
Done.

mohit_aghera → made their first commit to this issue’s fork.

This seems a valid issue.
I am able to reproduce the issue.

I believe since description field is widely used, we can't make additional changes apart from tweaking field formatters and using inline_template to render the description of file description field.

I tried to reproduce the issue while doing triage for Bug smash initiative.

Thanks to @rcodina and @dieterholvoet for providing hints about the project.
It seems to be happening when file_temp_path setting isn't correct one.

Since this issue happens due to missing $settings['file_temp_path'] setting,
I added following config in the settings.php file.
$settings['file_temp_path'] = '/abc';
This directory doesn't exist at all inside my docker container and system is not able to create it either.

After doing cache clear, I was able to see the theme properly.
I noticed following message on the screen which were able to convey that file_temp_path is incorrect.

Notice: tempnam(): file created in the system's temporary directory in Drupal\Core\File\FileSystem->tempnam() (line 274 of core/lib/Drupal/Core/File/FileSystem.php).

Warning: file_put_contents(temporary://filebNGCln): Failed to open stream: "Drupal\Core\StreamWrapper\TemporaryStream::stream_open" call failed in Drupal\Core\File\FileSystem->saveData() (line 498 of core/lib/Drupal/Core/File/FileSystem.php).

When we don't have the settings $settings['file_temp_path'] = '/abc'; in settings.php, it falls back to `/tmp` directory of the OS.
In the following snippet, we can see that if settings is empty, Drupal will pick the operating system's tmp directory.
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

I think we would need more detailed steps to reproduce the issue.

I confirm that I’m interested in serving as co-maintainer and have read the required documentation, including the sub-system maintainer guide.

Thank you for the opportunity and all the support!
I truly appreciate it!

I believe this issue doesn't exist anymore.

Reasons:
Right now we are displaying file upload help text like following image.

The value 8 MB is coming from negotiation between what we defined in field setting and post_max_size & upload_max_filesize php.ini variables.
Refer https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

Once we have the minimum file size upload value from above php.ini variables, we compare it with the value defined in field setting's file upload size setting.
See https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/file/... for validator callback and for https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/file/... actual callback implementation.

Here we decide what maximum file size should be allowed for users to upload and we set in validation constraint.

// Cap the upload size according to the PHP limit.
    $maxFilesize = Bytes::toNumber(Environment::getUploadMaxSize());
    if (!empty($settings['max_filesize'])) {
      $maxFilesize = min($maxFilesize, Bytes::toNumber($settings['max_filesize']));
    }

So essentially, user will always have the minimum possible file upload size and it will be calculated based on our settings and php.ini

With this there is no need to show different message to users as it might be too technical for them.

Feel free to reopen the issues if you notice further issues.

This seems to have been refactored and we are checking if file exists based on wrapper.

https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/file/...
Following is the latest code:

if (!file_exists($file->getFileUri())) {
            if (!$this->streamWrapperManager->isValidUri($file->getFileUri())) {
              $this->logger->warning('Temporary file "%path" that was deleted during garbage collection did not exist on the filesystem. This could be caused by a missing stream wrapper.', ['%path' => $file->getFileUri()]);
            }
            else {
              $this->logger->warning('Temporary file "%path" that was deleted during garbage collection did not exist on the filesystem.', ['%path' => $file->getFileUri()]);
            }
          }
          // Delete the file entity. If the file does not exist, this will
          // generate a second notice in the watchdog.
          $file->delete();

Here if file doesn't exist, we are adding errors in the log.

Do we explicitly need to add a new try-catch now?

MR is green now.
Moving back to NR

I've updated the title.
I did a search and didn't found additional instances except one kernel test which we can't update

grep -r 'implements FormInterface' core/modules/file/tests

The code mentioned in the issue summary has been refactored.
We don't have $this->assertSession()->waitForElement('css', '.messages--error'); assert statements.

It's been refactored into more general methods.
See https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/file/...

Feel free to reopen the issue if you see further issues.

Closing this for now based on inputs from @kim.pepper

Feel free to reopen it if you notice further issues.

griffynh → credited mohit_aghera → .

smustgrave → credited mohit_aghera → .

I checked the gigantic thread of the issue #2669074: Convert file_create_url() & file_url_transform_relative() to service, deprecate it →

The changes approximately starts from comment 83 https://www.drupal.org/project/drupal/issues/2669074#comment-13003933 →

I don't see any specific discussion of switching generateString instead of generateAbsoluteString to FileUriFormatter.

I believe this issue might not have caught till date because there might be very few sites who are using URI formatter.

I've updated issue summary as well. Happy to close if required.

mohit_aghera → made their first commit to this issue’s fork.

This certainly seems a regression because of issue #2669074: Convert file_create_url() & file_url_transform_relative() to service, deprecate it →

As per the change record file_create_url()<code> is supposed to be moved to <code>\Drupal::service('file_url_generator')->generateAbsoluteString($uri)

Instead of that, it is updated with $this->fileUrlGenerator->generateString($uri)

That sounds the root cause of the regression.
Probably this wasn't caught becuase we don't have test coverage for this one.

I've raised a PR with the fix and working on test coverage.

mohit_aghera → made their first commit to this issue’s fork.

I was trying to reproduce the issue and had no luck.

I tried following instances.
- In core check if there are any concatenated fields like "description_field_required"
Initially I did a quick search to see if we have dynamic variable. i.e. grep -r "_required'" core/
This didn't yielded any results.

Later I did same search with prefix i.e. "description_".
This didn't returned anything either.

My only guess is that if the patch from https://www.drupal.org/project/drupal/issues/2320877 ✨ Add a setting to make description a required field for file items Needs work issue is applied to core.
In-case it is not applied cleanly then $field_settings might not have key "description_field_required"
Reason is that, the error appears on line 249 which is close to the change in the above PR.

Another reason can be that above patch is used and update hook isn't called.
There is a "file_post_update_description_required()" which sets the values.
To confirm that there is a dedicated update test as well https://git.drupalcode.org/project/drupal/-/merge_requests/5876/diffs#di... which checks that "description_field_required" doesn't exist before update hook and it exists after update hook.

Apart from that I am in favour of postponing the issue unless we have clear steps to reproduce.

Please reopen again if you feel it is happening again with majority of users with the vanilla core.

- Updated a new kernel test to validate the entity query results.
- Using other general practices for creating fields etc.

Code is already fixed so no changes are required in EntityReference handler plugin.
Hiding existing patches in favour of MR.

Tests are passing on local.

mohit_aghera → made their first commit to this issue’s fork.

Moving to needs review since "needs-review-queue-bot" falsely made it to needs work
PR is green and it contains all the necessary tests.

- Added a test to validate that attribute passed is exist on the render.
- Variable hint in the twig template.

mohit_aghera → made their first commit to this issue’s fork.

Ahh, apologies for the confusion.
I mean there won't be any errors now since it is handled in D8 core properly.

We can merge the test as it is passing on the local.
I've triggered the job again and will update the issue once it is green.

I believe we should close this issue.

There are few more usages of upload validators with the file elmement.
like
Drupal\locale\Form\ImportForm.php

$form['file'] = [
      '#type' => 'file',
      '#title' => $this->t('Translation file'),
      '#description' => [
        '#theme' => 'file_upload_help',
        '#description' => $this->t('A Gettext Portable Object file.'),
        '#upload_validators' => $validators,
      ],
      '#size' => 50,
      '#upload_validators' => $validators,
      '#upload_location' => 'translations://',
      '#attributes' => ['class' => ['file-import-input']],
    ];

Feel free to re-open if you feel we still need to alter documentation to mention both the #types of fields.

I believe it is intentionally that file entities don't have canonical route.

As @berdir mentioned in comment https://www.drupal.org/project/drupal/issues/2345761#comment-13180034 →

files will IMHO remain second-class citizens and we will not introduce an actual canonical route in core, and we decided against having one that points to the physical file.

I'm tagging for sub-system maintainer review to get second eyes.

I am not able to reproduce the issue in vanilla Drupal core.

I did following steps to reproduce:
- Use article content type
- Add file field with "Description" field enabled in file widget
- Set cardinality to unlimited.
- Create new article and add one file with description.
- Save article.
- Edit article again and add new file.
- Ensure that first file's description remains unchanged

Besides above steps, I was able to see all the values in $items of Drupal\file\Plugin\Field\FieldWidget\FileWidget's formElement method.

Feel free to reopen if you notice additional issues with the updated steps to reproduce.

I implemented the changes in the recent PR.
I haven't added tests for constructor deprecations considering it is not required here https://www.drupal.org/about/core/policies/core-change-policies/how-to-d... →

I didn't found many uses outside of core https://git.drupalcode.org/search?group_id=2&page=2&scope=blobs&search=%...

Only a few instances in the contrib space.

mohit_aghera → made their first commit to this issue’s fork.

Added tests to cover the scenarios mentioned in the description.
Test is green now.

I was thinking about this earlier and the parallels to AccessResultInterface. The equivalent here would be AccessResultForbidden for -1, Neutral for NULL and Allowed for the array return. Having a similar interface would properly restrict what can be returned from this hook.

This totally makes sense to me. I can refactor this one.

mohit_aghera → made their first commit to this issue’s fork.

Tagging for initial review with the accessibility team.
I'll work on issues depending on the inputs from the team.

I'm in favour of using "aria-required: true" unless there aren't any other ways.

Currently when field is required, we have following markup in claro in standard profile.

Add a constant (possibly to FileDownloadController?) Might be named something like FILE_DOWNLOAD_DENY or (if on the download controller) DENY.

I've added the constant for the controller.

I initially though of using the Enum for this one, however since it was just one case, I didn't thought of using this.

Discuss whether the NULL "ignore" return value also merits its own constant.

I feel we don't need this one.
I did a quick check in core grep -rn --include="*.php" -E "const\s+\w+\s*=" core/ and didn't found any such instance.
Probably we've avoided in past?
Happy to do that if we want to go that route.

Refactor to use some generic access result mechanism. There might be limitations on what we can do in a minor, even with an upgrade path, but it is worth looking into.

@xjm I'm slightly unclear about this one.
Can you please provide more information.

mohit_aghera → made their first commit to this issue’s fork.

I believe this issue might not be required now.

Since this issue was added to increase the test coverage on file_field_presave function.
Since we have entire EntityReference mechanism happening inside FileFieldItemList, I believe this might not be required.

There is another class EntityReferenceFieldItemList which is responsible for loading the referenced entities has enough checks.

Considering that we might not need to add additional test case coverage.

For testing purposes, I’ve added an additional test to ensure that things goes well.
This test is passing on local.

mohit_aghera → made their first commit to this issue’s fork.

mohit_aghera → made their first commit to this issue’s fork.

Currently in Drupal 11.x, it still allows to provide folder path like "public://".
I believe we should add a validation to ensure that path doesn't contain URI scheme field in the path.

We can use the stream_wrapper_manager service to fetch all the schemes and validate that path doesn't contain string like "{uri_stream}://"

Hi @alan d

Thanks for raising the issue.

This seems to have carried over from Drupal 7.
Even on Drupal 11.x ImageItem plugin doesn't have the '#disabled' => $has_data (core/modules/image/src/Plugin/Field/FieldType/ImageItem.php line 200 or so)

File field settings form when field has data:

Image field settings form when field has data:

I believe this might have missed while porting to D8+.

Decide whether we should keep or not:
- I believe we should keep the#disabled attribute if image field has data.

If we consider the following scenario:
- In initial phase we mark image field as public file field.
- Upload a few files.
- Later change this field as private
- Expectation to editors will be they all the existing files are private however they are not.
So essentially if field has data then this option should be disabled.

I am tagging sub system maintainer review.

I don't have strong opinion on `ComponentType` vs `ComponentSource`.
However as non-native english speaker, I felt `ComponentSource` more convenient to realise that there are so x number of source plugin that allows XB component config entities to be created.

Regarding declaring source discovery:
Strong +1 from me.
I faced the issue while working on Pinto integration.
Since pinto object are standalone object, there was no easy way to identify and create Component config entities.
I'm relying on hook_cache_flush to refresh the list and generate new `Component` config entities.

mohit_aghera → made their first commit to this issue’s fork.

mohit_aghera → created an issue. See original summary → .

Updated code to use the config updater service.
There were unrelated functional javascript test failures. Triggered the job again.

mohit_aghera → made their first commit to this issue’s fork.

Attending

mohit_aghera → created an issue.

mohit_aghera → created an issue.

mohit_aghera → created an issue.