#Bug Smash Initiative

The IS is up to date now that we're keeping the wrapper.

This came up as a daily BSI

The issue summary appears incomplete and could use some love. Notably missing steps to reproduce.

Okay, thanks for your time. I'll fix the tests. To be clear, are there specific tests that need to be passed or all of them?
I am not sure if some tests may be failing due to something unrelated.

The MR now applies to D11.2.0, thx!

sorry, commnt mismatch.

- 3252288-50.patch is against 11.x
- 3252288-51_against_11.2.0.patch is against 11.2.0

Still NW because no test was added, but this patch applies on 11.x, from merge request https://git.drupalcode.org/project/drupal/-/merge_requests/12448

Patches #28 and #43 does not apply on 11.2 branch..

This has moved to \Drupal\media\Install\Requirements\MediaRequirements::getRequirements. It's also not in system.module so I'm updating the component.

I think now that this is in a much more isolated function we can use early returns for clarity. Something like this:

public static function getRequirements(): array {
  $requirements = [];
  $destination = 'public://media-icons/generic';
  \Drupal::service('file_system')->prepareDirectory($destination, FileSystemInterface::CREATE_DIRECTORY | FileSystemInterface::MODIFY_PERMISSIONS);
  $is_directory = is_dir($destination);
  $is_writable = is_writable($destination);
  if ($is_directory && $is_writable) {
    return $requirements;
  }
  $t_args = ['%directory' => $destination];
  $error = !$is_directory
    ? $this->t('The directory %directory does not exist.', $t_args)
    : $this->t('The directory %directory is not writable.', $t_args);

  // The rest of the inline template / description bits here.
}

Here's an alternative solution based on the end of #134 which is a bit easier now that we have the debug class loader (I think).

I copied the main test from https://git.drupalcode.org/project/drupal/-/merge_requests/11090 and it now passes with just the cacheability changes in https://git.drupalcode.org/project/drupal/-/merge_requests/12445

I think this is a lot more elegant, we can also add this to hook_entity_operation and hook_entity_operation_alter quite easily since hook BC won't be a concern afaik.

Keeping in NR to get a review of the overall approach, then I'll go ahead and do the hook changes as well.

to make a check against the database to see if any images are using the wrapper in the files_managed table.

Image styles may be generated for un-managed files as well (albeit not common), I don't see the files_managed table as a viable solution.

We should definitely see if we can at least warn users. I just don't know where/how that could obe done

We have had this subject come up for other issues in the past. Documentation has to start somewhere, this issue is as good as any other to do so, stick it somewhere in the README, or some "deployment guide" for Site Owners to note that '/styles/' is a reserved path on all streamWrappers.

Potentially a feature to be able to select where images styles may be located could be a solution.

✨ Split ImageStyle into the config entity and a separate event-based image processing service Needs work would be the best for this, it would allow solutions for numerous concerns. The new FlushEvent it proposes likely could allow this to be handled per streamWrapper by later changing the streamWrapper spec to require each streamWrapper provider listen and perform the flush as needed.

@generalredneck I agree, sorry if it sounded like I was discarding the idea that this could be a bug. We should definitely see if we can at least warn users. I just don't know where/how that could be done

Yes it's an edge case, but with real world application on an education site i maintained.

My proposal was a single query per wrapper. I don't know how that looks in the code after 3 years, but I feel that the performance implications haven't been fleshed out. Also since it's been a problem since drupals in inception doesn't make it correct... But possibly an acceptable bug/problem.

At the very least we document and/report on the problem. Thoughts?

This popped up in bug smash today. I'm not really sure how we'd go about fixing this, but it does seem like a fairly big edge case.

I'm not sure the db query idea would be viable for performance, it looks like flushing image styles in all stream wrappers has been a thing since D7 and probably earlier.

@acbramley Okay 'opening separate browser tabs' might be I was thinking of a nightwatch test. I'll have another review of the latest. My suggestions were nitty.

Hi @oily

Thank you for trying to improve the documentation but it now exceeds the 80 character line length and I don't think the extra words actually clarify it any better. "a node" implies it is a single node, and we are not opening separate browser tabs as that's not possible. We are simulating a similar scenario to what happens in a browser by previewing 2 translations in a row and then going back to the edit form of the first translation. IMO this is all explained clearly in the current doc block and inline comments.

Edit test comment for greater detail/ clarity.

#5 also worked for me. Drupal 11.2

Thanks for fixing those, seems we are getting test-failures now, re-ran twice

I resolved the Drupal 11.2 conflict.

And here's a patch for 11.1.8.

catch → closed merge request !8877

Thanks for double checking.

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

Actually we don't have any rules about this, but I think I prefer PositiveOrZero, just because it's more expressive compared to Range: min: 0.

Closing this as a duplicate of 📌 [meta] Replace lazy service proxy generation with service closures Active which will change all of core's proxy classes to use service closures.

Moving this to Claro theme as per #26 - IMO this could be closed.

I originally thought I couldn't reproduce this but it was because the alt field was required on the image field I was uploading to, so the initial preview submission wouldn't go through because the alt text was required, then it needed another submission to go through. When returning in that case the image was restored.

However, if alt text is optional, I can reproduce it. I've debugged into NodeForm::form and confirmed that the form state's entity does have a value for the file and the files exist in the database and on disk so I don't know if this is a Node issue.

Could it be an issue with the image/file widget? Note that I can reproduce it with a File field too.

Bumping this again to see if anyone feels strongly about this? I would prefer to keep it by revision id.

Closing out due to lack of activity.

What about this in the 3.0.x branch instead

We have a simple select for "new" and "search for existing book" if they select the 2nd then an autocomplete field appears regardless of how many books there are?

Finally getting around to this one, I think for now we just return NULL and don't automate the entity upcasting because I can't think of a good way that you could safely upcast an entity parameter if it's a union type?

Perhaps only if all of them are a subclass of $entity_type->getClass();?

This issue has an inverse effect as well, namely that when a new file is uploaded to an unpublished revision on a previously published entity, that file is permanently access denied.

Example: A draft update to some node has updated the file, and the reviewing user wants to check the file contents before marking the revision as published.

This is caused by the fact that the published revision is loaded when loading file references in `file_get_file_references()`.

There is even a comment documenting this.

// Iterate over the field items to find the referenced file and field
// name. This will fail if the usage checked is in a non-current
// revision because field items are from the current
// revision.
// We also iterate over all translations because a file can be linked
// to a language other than the default.

The same error appears when generating a proxy class for a class with union types used in definitions.
php core/scripts/generate-proxy-class.php '\Drupal\Core\Batch\BatchProcessor' "core/lib/Drupal/Core".
Affected me working on 📌 Create an interface and initial class for the batch processor service Needs work

Answered, back to RTBC

Hm, over in 📌 Add validation constraints to contact.settings Active we just committed a bunch of changes with

      constraints:
        Range:
          min: 0

Is PositiveOrZero preferred? If so, why? There is no discussion of adding it in this issue. We should be consistent, in which case should we update other cases of Range with a minimum of 0?

MR

https://git.drupalcode.org/project/drupal/-/merge_requests/10289/diffs

Moved the config translation hook over to the NodeHooks file and removed the additional NodeLocalTaskHooks.
Fixed some lint errors.

https://git.drupalcode.org/project/drupal/-/merge_requests/10289

Left comments on MR

catch → closed merge request !6248

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

This came up as a daily BSI target but see it's already been tagged.

Appears to still be valid, from what I can tell.

Since there's been no follow up going to close out. If still a bug please re-open updating the issue summary

Thanks.

I re-rolled the contents of the MR over the 11.2.x branch. Currently fails to apply over 11.2 release.

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

catch → closed merge request !982

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

smustgrave → credited xjm → .

@xjm for 🐛 Multiple add links in breadcrumb when creating the translated node Active and 🐛 "Add media" causes error in Firefoy, not in Chrome Active

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

Also affects XB, in ✨ Define JSON Schema $refs for linking/embedding videos and linking documents Active . For now, it's working around it by overriding FileItem::generateSampleValue().

One question on the MR.

catch → closed merge request !9319

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

Rebase.

Test-only test fails as it should. Tests are green.

Added further suggestion to review. My review is based on my understanding that the sequence in the test is as follows:

1. Edit same node in two tabs, one is english translation, the other spanish
2. Change the title in one of the tabs eg english translation
3. Preview the spanish version of the node in other tab
4. Check whether the spanish version title has been changed to the new title in the english version

Nitty rewording of one code comment. Run test-only test.

Amending attribution.

Crediting myself for mentoring and scope guidance in #22.

acbramley → credited larowlan → .

Crediting @larowlan for helping debug the language prefix issue with toUrl().

This is ready for a review, I'll create a CR once we agree this is a decent solution.

xjm → credited star-szr → .

Adding missing credits for the major issue triage as per:

@webchick, @catch, @Cottser, @effuglentsia, and I agreed that this is a major issue

Thanks!

xjm → credited effulgentsia → .

xjm → credited star-szr → .

xjm → credited tim.plunkett → .

Adding missing credits for issue triage per #20:

@tim.plunkett, @effulgentsia @alexpott @xjm @cottser and @dawehner discussed that.

For those posting comments more recently, this issue was closed over a year ago, and as a result, maintainers will generally not see your replies. You're better off opening new issues in the core queue referencing this one. Thanks!

I skimmed the comments and didn't find unanswered questions. On reading the MR I made one suggestion to a comment and left some questions, all pretty simple. I would usually just accept the suggestion but since there were some questions I left that for now. Because of the questions, setting this to NW.

acbramley → changed the visibility of the branch 2907091-previewing-translatable-nodes to hidden.

Postponed as per #36

Added coverage in ScanDirectoryTest.

xjm → credited larowlan → .

Saving a couple missing credits from the sprint.

.

.

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

I've started a new branch here because the existing MR did not fix the issue for me and was quite out of date.

The issue is that the temp store uses the node UUID as the key, and is loaded verbatim regardless of language.

Changing the language on the back link won't fix that, in fact the back link already contains the correct langcode in my testing.

The whole hook system has since changed.

This is how hooks worked in procedural land.

We intentionally preserved that with attributes (though it's under discussion)

I would presume we'd preserve that as well if these mive to oop too.

This might not belong with the extension system, but sure exactly where it should be though.

Amending attribution.

Amending attribution.

The tests for scanDirectory are at \Drupal\KernelTests\Core\File\ScanDirectoryTest

Restoring attribution.

Amending attribution.

Just one minor comment on the MR, I think its ok, but would be good for a second opinion

This came up as a daily BSI target.

Think we will need a test case showing the issue.

smustgrave → credited mstrelan → .

Myself for 🐛 When there's non-zero latency, saving the Settings Tray causes the plain Toolbar to be visible first Postponed: needs info

@mstrelan for 🐛 User context missing when using toUrl in some circumstances Needs work

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

I think the one point from #93 that is really worth working out is the priority point.