#Bug Smash Initiative

Thanks for working on this, left a suggestion

I'm running into a similar problem on 10.3.2. However, the suggested solution does not work for me, though my use case is perhaps slightly different.

I am using core's included Block Content Permissions, but I have chosen not to give any roles "Create new block content" permissions for my various custom block types and this is preventing media from being uploaded to fields on those blocks in Layout Builder. Granting those permissions does fix that and allow media to be added to the library within Layout Builder blocks, however, it also allows users to add new instances of these custom block types under '/block/add', which I do not want. I only want those users to be able to add those blocks in Layout Builder.

The "Create new block content" permissions are not required in order to actually add these custom blocks in Layout Builder at all (which I believe is how it's supposed to work), but they are required in order to add media to fields within those blocks. So I believe this is a variation of this same problem, which is that Layout Builder is using faulty access checks when uploading media to custom blocks.

I'm not sure why the solution in #37 does not work for me.

This was open for 10 years and it was changed to a task bit it's imho unclear what the task would be.

Let's just close it?

I agree with #18. The error message is very useful for developers. I don't see what should be changed to make this any better.

It's in 10.3.

HI @heykarthikwithu , thank you for the mr.
I'm not sure why scope is being reduced to link or src tags, when this also affects meta tags, for instance.
Regards.

Tests are now passing, moving to Needs Review.

To figure this out I would like to see the browser output from the test. However, the artifacts for the test-only changes test run does not include the browser output. Is this by design? Is there something that can be changed in the template so the browser output is saved?

To get the browser output I made an MR with just the test changes. And looking at all the pages of output hasn't given me an idea as to why the fail test passes in gitlab but fails locally.

As it needs test, and IS needs updating and the added test doesn't fail on test only right now this is still needs work.

Also as far as I can see the use of titles on links is largely discouraged

@alexpott This issue is about the default link text, not about link title attribute.

I've read the issue summary and I've not seen a use-case for providing a default link title without the link url.

@alexpott We are using this patch in many sites. The client needs to set a default link text so it is always the same text and the URL is changing: let's say for example "See the full source for more details", where the link pointing to the resource is different in every node.

this does result in a behaviour change and it will be confusing for users because once you have a default link title it's completely not obvious that you could remove the link title to save the content without a link.

The way I see this, more than "this does result in a behavior change", is that this actually allows something expected, the ability to save a default link text (instead of the frustration of doing it and see that it is not saved), and as you say, allowing the expected functionality allows the situation of a new node with a link text already filled and the user could be kind of forced to fill either the URL or delete the link text for that specific node.

However having to delete the link text to leave it empty so it passes validation without a URL in a non required field scenario, is something core is doing right now, not this fix. This fix is just removing a veil over an existing problem in core, hidden by the bug of the default text not actually being saved, despite the UI's supposedly allows it as with any other field. That's why I am encouraging to open a new issue about this, tackling what core does in the form display with links and URLs not filled on non required fields.

This suggests a potential fix - only allow setting a default link title if the field is required.

While it could be the most common scenario, we would not be satisfying all possible desired functionalities. I, as client, could perfectly desire to have a default link text for this field in all nodes that have an URL, which maybe is the 80% of the nodes for that content type, but not 100% so I don't want it to be required, but the default link text is a must, for consistency and time saving.

The real problem here is that, right now, core forces you to remove the link text if you write something on it and click save without filling the URL. This is the issue we are talking about now, the one unveiled by fixing this bug, +8 years open issue.

A possible solution to current's core not very good behavior, could be maybe to let the user have clear that the link text is ultimately dependent on the URL on a specific node edit/create form. So for example, if the link text input would be hidden and only be shown when the URL is actually filled, that would make things clearer, and possibly we wouldn't have to make the user delete the link text input if she/he finally saves the node without a value for the URL, as core currently enforces. As @quitetone says, that would require an usability look.

My proposal is to open a new issue from here to improve current's core confusing behavior, instead of leaving this bug unfixed forever so we not have to look at what is actually an incomplete implementation. If we decided not to fix this bug until we have a better display form implementation we should at least alert the user than if she/he takes the time to fill a default text link value, she/he should also fill an URL default value, or directly remove the option of filling a default text link. But I would love not to do these things, so many people as myself could at least be patching (and maintaining, rerolling) this to allow this necessary feature to our clientes. Many people are actually needing to have this failing feature solved.

I added a Functional test that shows the error locally. But when I run the test-only changes it doesn't fail as expected. Gotta run.

On posting the comment I do think we could consider a completely different fix. We could only allow a default link title to be set if the default link is also set. I've read the issue summary and I've not seen a use-case for providing a default link title without the link url. The link title and URL have to be related right - so the user needs to make the title fit the link.

Also as far as I can see the use of titles on links is largely discouraged - see https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/titl..., https://www.tpgi.com/using-the-html-title-attribute-updated/ and https://www.24a11y.com/2017/the-trials-and-tribulations-of-the-title-att... I don't think providing a default title with no related link is going to make this situation better.

@juanolalla this does result in a behaviour change and it will be confusing for users because once you have a default link title it's completely not obvious that you could remove the link title to save the content without a link. This suggests a potential fix - only allow setting a default link title if the field is required.

I'm not sure about the fix here. I think saving an invalid default value might result in more problems as all other default values are valid and not empty. I wonder if we should consider a different fix where we save the default title value somewhere else in the field config if the link is not present. That way we wouldn't have to override extractFormValues which feels a bit risky.

I think it would be easier to introduce a field item list class for the link field type and override \Drupal\Core\Field\FieldItemList::defaultValuesFormSubmit() to manually return a default value if the title setting is set to required and there's a value for the link title.

I don't understand how that would solve the current problem of the UI's bug, failing to save the default value in the field settings form. And why are you changing the status of this bug fix to "needs work" again.

And this fix does cause changes the UI, on a node/add form.

No, this fix doesn't cause changes in the UI. What you are referring is not part of this fix, is Drupal core current's confusing UI.

Right now, if you want to save a default value for the link text, that's broken. This patch is allowing many sites to fix that, for years. It should be committed.

The URL part of link field will be shown to the user as required.

This is the current core behavior, not the result of this fix. If you try to save a link field with text and no URL. Please open a new ticket to improve that.

Related/potential duplicate that addresses this more broadly: 🐛 Not all URIs are URLs, but UriWidget won't accept non-URL URIs Needs work

I think it would be easier to introduce a field item list class for the link field type and override \Drupal\Core\Field\FieldItemList::defaultValuesFormSubmit() to manually return a default value if the title setting is set to required and there's a value for the link title.

There shouldnt be any validation of anything after `tel:` to be honest. Theres simply to many localized variants, take a look a look a https://github.com/giggsey/libphonenumber-for-php which is implements the PHP version of the unofficially standard phone number validation and formatting library libphonenumber.

I think, non-empty-string is the only thing we should be validating against.

Re this issue as a whole, things would be a hell of a lot easier if we don't consider tel:

Update: we also hit the webform submission issue (thankfully from our tests) as mentioned in comments 163 - 179
I created an issue in webform and proposed a fix there 🐛 Incorrect use of 'render element' for webform_submission_data Active

Yes, I read #135 and the other comments as well. Yes, this is a bug with a straightforward fix. That fix, however, has caused unexpected behavior as noted in #79, #101, #116, #121, #125 and #143.

And this fix does cause changes the UI, on a node/add form. The URL part of link field will be shown to the user as required. That is the change and yes it make sense. However, there is no information provided to let the use that the form can be submitted without completing the required field. That is why I asked for a usability review here 3 years ago and I still think one should be done.

So please ignore my patches from #11 and #12

i made a MR against 11.x-dev based on the patches

With the settings shown in the screenshot, before this change the link field is not required and after this change it is

That is not true, please see #135 for the explanation. This fix is not changing at all the current's Drupal core behavior, that requires an URL if you would like to set a link just with a link text.

This is just a simple fix for the existing bug on the field settings form: the default link test it is not being saved.

This fix has nothing to do and touches nothing of the display form. Please open a new ticket to discuss current Drupal's behavior from an usability point of view, and let this fix solving the bug be committed.

With the settings shown in the screenshot, before this change the link field is not required and after this change it is, even though the 'Required field' setting is not selected. I think that will cause confusion for admins and content editors, as it has done for people working on this issue. I think we find a way to resolve that here. For that getting an Usability review will help, so I am tagging and changing status.

Hola, this came up in the #bugsmash channel as the PMNMI daily triage target.

Since we haven't had an update in nine months, if there is no update in the next three months, this issue may be closed.

This is ready for review again after a rebase + some changes. Tests pass green.

Just ran into this in 10.3.x with views on the USDOJ website where the pager would sometimes not render without a discernable rhyme or reason. Sure enough, adjusting the pager ID to 99 ensured that there weren't issues.

I don't have steps to reproduce from scratch, nor the liberty of time to faff around with reproducing this, but the pager ID collision does appear to be something that can still happen.

Stands to reason you could perhaps write a test for this that has a view rendering content, some of which has a pager on it, and both using the same pager ID of 0, but the view in the content pager is empty, which in turn bubbles up to the overall view pager ID.

I wish there were an easy solution such as setting pager ID to the views limit + 1, but if the overal view is 25, and the view in the content displaying in the view is limited to 25, you'd have the same pager ID collision again.

Why does pager ID need to be numeric anyways? If it could be alphanumeric you could go view-name_dislay-name?

✨ Additional blob field sizes in schema definitions Needs work is now in RTBC. If it lands before this, I could easily see us switching from a blob:big to blob:medium.

I´m sorry for just rerolling this patch in a deprecated way ... here is a rerolled patch for 10.3.10

Hello there.
This might be related: https://www.drupal.org/project/drupal/issues/3442910 🐛 Error "Non-reusable blocks must set an access dependency for access control." with layout builder and media library Active
I hope it helps.

rebased the MR to bring it up to date with 11.x

@TimeFor what a life saver. That was exactly the issue for me. I moved to a new db and watchdog kept breaking with cache errors.

Error: Call to a member function getCacheTags()
Error: Call to a member function getCacheMaxAge()

Sites using Drupal 11 and MariaDB must use 10.6 or greater. This was set in 📌 Update INSTALL.txt and hook_requirements() etc. with remaining Drupal 11 platform requirements Fixed .

Therefor closing as outdated

I encountered such issues with Media but this could also solve issues such as 🐛 [regression] Entity view block not displayed after #2962166 Needs review since it applies to all entity types.
Finally a true recursion detection and using pre/post_render callbacks to do it is very interesting.
I tested it on one of our projects and it works great, thanks!

The latest MR fixed the issue on my end: I could not set the aggration settings to "Count Distinct" which was breaking aggregation in my view all together. After the patch, it all worked.

Thanks everyone for the great work on this so far.

#64: The documentation → as such indicates that tel: should be supported and #11 is correct that PHP's native parse_url is not fit for purpose here. I think we need to either revive the custom approach or do another scan to see if there's a library out there now which supports this requirement.

When implementing this, please don't forget use cases outside of the USA. In Australia, we have some shorter six digit numbers (132 500, for example) and there are also the emergency service numbers (not necessarily 911 - alternatives I know include 000, 111 and 112.

Whoops, no need for this to be postponed anymore.

For what is worth it, I've tried the index mentioned in #34 → and it improved a lot the performance of the query.

Before the index:

51 rows in set (57.57 sec)

After the index:

51 rows in set (17.26 sec)

The index query for reference: ALTER TABLE file_managed ADD INDEX groupidx(fid, filename, filemime, filesize, status, created, changed);

And this is the select query: SELECT file_managed.fid AS fid, file_managed.filename AS file_managed_filename, file_managed.filemime AS file_managed_filemime, file_managed.filesize AS file_managed_filesize, file_managed.status AS file_managed_status, file_managed.created AS file_managed_created, file_managed.changed AS file_managed_changed, SUM(file_usage_file_managed.count) AS file_usage_file_managed_count, MIN(file_managed.fid) AS fid_1 FROM file_managed file_managed LEFT JOIN file_usage file_usage_file_managed ON file_managed.fid = file_usage_file_managed.fid GROUP BY file_managed.fid, file_managed_filename, file_managed_filemime, file_managed_filesize, file_managed_status, file_managed_created, file_managed_changed ORDER BY file_managed_changed DESC LIMIT 51 OFFSET 0;

I think you missed core/modules/menu_link_content/src/Plugin/migrate/source/d7/MenuLinkTranslation.php, so NW for that.

Thanks for catching that. Rebased the MR and added the change.

Can we really make the existing trait a wrapper for the new trait in the migrate_drupal module and not add any deprecation notice?

Not sure what the appropriate deprecation message, given #29 🐛 Move content_translation I18nQueryTrait to migrate module Active and whatever is going to happen to migrate_drupal, but I think I agree with the idea that the deprecation message should be set when migrate drupal moves out of core.

Can we turn these STR into an automated test? I think so. I am adding the tag for tests.

Unfortunately, this is more difficult than you might think, because all classes are autoloaded when tests run. So the trait would never be missing, unless it just doesn't exist at all.

I did run the steps to reproduce with the Entity Import module. I had to

• Use the lenient composer plugin
• Edit entity_import.info.yml
• Add an inherited method return type.

I was able to install entity_import against 11.x and this MR's branch. Confirmed that the fatal error was reproduced on 11.x and gone after the trait was moved.

Moving back to Needs Review. Leaving at needs tests in case there is another idea of how to test this proposed.

I think you missed core/modules/menu_link_content/src/Plugin/migrate/source/d7/MenuLinkTranslation.php, so NW for that.

Can we really make the existing trait a wrapper for the new trait in the migrate_drupal module and not add any deprecation notice? Maybe we can, but it feels too easy. I guess the idea is that when we deprecate the new trait, that will automatically deprecate the existing one (the wrapper), and we have to do that before the next major version (D12).

The existing steps to reproduce (STR) involve a contrib module. That has two problems:

1. It is not clear from the STR that this is a bug in Drupal, rather than the contrib module.
2. Manual testing is complicated, since the fix is on a branch based on 11.x and the contrib module does not have a release compatible with Drupal 11.

I know, I can add the issue fork for the D11 compatibility issue as a package repository and install the contrib module that way, or I could use the lenient composer plugin. I said it is complicated, not impossible. ;)

We can solve both problems if we give different STR. Using drush php,

> \Drupal::service('plugin.manager.migration')->createInstance('d7_custom_block_translation')->getSourcePlugin();
PHP Fatal error:  Trait "Drupal\content_translation\Plugin\migrate\source\I18nQueryTrait" not found in /var/www/html/core/modules/block_content/src/Plugin/migrate/source/d7/BlockCustomTranslation.php on line 22

Fatal error: Trait "Drupal\content_translation\Plugin\migrate\source\I18nQueryTrait" not found in /var/www/html/core/modules/block_content/src/Plugin/migrate/source/d7/BlockCustomTranslation.php on line 22

Before running that code, I installed Drupal with the standard profile, enabled migrate_drupal and a custom module with a simplified version of d7_custom_block_translation:

id: d7_custom_block_translation
label: Content block translations
source:
  plugin: d7_block_custom_translation
process: {}
destination:
  plugin: null

The original version of d7_custom_block_translation is in the content_translation module.

Can we turn these STR into an automated test? I think so. I am adding the tag for tests.

We should update the issue summary with the improved STR. I can do that, but not now. I need some sleep. For now, another tag.

When I check out the feature branch from the MR, it fixes my manual test:

> \Drupal::service('plugin.manager.migration')->createInstance('d7_custom_block_translation')->getSourcePlugin();
= Drupal\block_content\Plugin\migrate\source\d7\BlockCustomTranslation {#7934}

It would be nice to get some manual testing of the original report: does the current MR fix the problem with the contrib entity_import module? Or does that require moving a lot more into the migrate_drupal module?