🇮🇳India @mohit_aghera

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.

mohit_aghera → changed the visibility of the branch 11.x to hidden.

mohit_aghera → changed the visibility of the branch 2637808-languageformatter to hidden.

mohit_aghera → created an issue. See original summary → .

mohit_aghera → created an issue.

jjchinquist → credited mohit_aghera → .

smustgrave → credited mohit_aghera → .

Fixed all the phpstan issues.
Resolved merge conflicts and removed changes related to views as this wasn't necessary.
There is one more failure in the CI, I'm going through that.

I've hidden the patches in favor of MR approach.

I evaluated two solutions for the issue.

1. Modify/cleanup the views in hook_modules_uninstalled hook
2. Implement a new module uninstall validator for the display handler plugins Comment #19

Modify/cleanup the views in hook_modules_uninstalled hook
- I feel this might not work in this case.
Reason is, that we can't fetch the list of all the display handler plugins provided by the module being uninstalled.
In the uninstall() function, this hook is the last one to get called https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

Since plugin ID or anything won't be available here, we can't identify which display handler plugins we want to unset from views config.

I evaluated another option to check the plugin by doing namespace-based lookup, however, that seemed a bit hacky to me.

Considering that, it might not be straightforward to update all the views and unset the display handler plugins.
We can certainly disable the plugin in views > Advanced config settings.

Implement a new module uninstall validator for the display handler plugins Comment #19
This worked fine and prevented module being uninstalled.
I added on validator plugin along with kernel test to validate the exception.
This seems to be working fine.

I feel at this point we should go ahead with the approach where we display a warning rather than silently updating the views config.

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

@freelock, thanks for providing the detailed steps to reproduce.
I'll update the tests and will update the PR.

Can we close the other one and keep continue working on this issue itself.
We can carry steps to reproduce.

I did tried to reproduce the issue based on steps mentioned in issue summary.
It seems issue doesn't exist anymore.

I even tried to follow based on step #23 and it seems to be working fine.
I created the view added in patch #16 and added the block in the layout builder for user entity.
It worked fine.

@possibri, @laurentwh
If you noticed the issues, please provide a test view or something I can use to reproduce the issue.

We probably need updated steps to reproduce or see if it has been addressed in recent 11.x cycle.

I could reproduce the issue with the steps mentioned in the issue summary.
We can replicate in vanilla Drupal with just a path module of Drupal core.

I've updated the PR with the latest 11.x and made a little bit of refactoring code improvement fixes.

There are a bunch of test case failures, I'm working on those.
Hiding other patches since MR is the preferred approach.

@ckng
I see you are following the correct steps.
Maybe some other module might affect the outcome?

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

Hi @xjm
I've fixed the following issues from comment #19

• Point 1
• Point 2

Comment is updated for both the cases.

Regarding point 3:
- I've added the new test that validates both the points.
Fixed few more things in the MR

About the custom_url option:
- Primarily it is visible on the block plugin display and other non-page display.
So I believe we should hide it from the page display?

Probably we can use the inputs from the sub-system maintainer.
This might cause confusion to people as `More` link on the page display might not make sense.

I notice that there is no option to configure the link, it just allows to enable/disable link.
Which is correct if we want to keep option.

I'm happy to walk you through the views setup, feel free to ping me in #bugsmash.

Regarding #20
I agree with you for this one.
We can disable it from the page display and keep it active for block display.

Hiding the patches since we have a MR now.
Keeping issue in needs works since there are a few unrelated test failures that I'm looking into it.

Tests added for permissions https://git.drupalcode.org/project/webform_revision_ui/-/merge_requests/....
PR is green now.

mohit_aghera → changed the visibility of the branch project-update-bot-only to active.

mohit_aghera → changed the visibility of the branch project-update-bot-only to hidden.

Thanks for the rebase @mrshowerman

I've fixed the phpstan issues and PR is now green.

I think we are good to get another round of review since all the issues are addressed on the PR

mohit_aghera → changed the visibility of the branch 3107993-3x-template-not-found to active.

mohit_aghera → changed the visibility of the branch 3107993-3x-template-not-found to hidden.

mohit_aghera → created an issue.

I've addressed the feedback in #40
I think node tests aren't required as such.

Fixed a few more things in the PR
- convert it to use OOP hooks
- Fixed a few things in tests.
- Tests seems green now.

Hiding all the patches in favour of MR.

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

@larowlan All the feedback mentioned in the PR is fixed now.
PR is green, I think it is ready for another round of review.