🇪🇸Spain @juanolalla

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 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.

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.

There are no interface changes with this issue, it's just a bug: the link text was not saved, and now with the fix it is saved.

So this doesn't need usability review.

No remaining tasks and nothing else, so moving this to RTBC.

From #125 🐛 Default value for link text is not saved Needs work :

The patch mostly works. However, if a default title is provided, when trying to save the entity form the URL field will be required, there's no way of saving the field without an URL. Which is what @csakiistvan said in #121.

From #121 🐛 Default value for link text is not saved Needs work :

For me this patch is is very nice, but the link field will mandatory on the form display (but my field is not mandatory on the field settings.)

I see there is still some confusion with the natural core's behavior of the form display, forcing an URL to be set if there is a title set, which is correct, and the issue we are fixing here, just allowing a default title text without a default URL in the field settings form.

We had that explained in #82 🐛 Default value for link text is not saved Needs work :

The review in #79. is based on an incorrect understanding of the requirements.

Expected Results:
# User should be able to Save the node without mentioning the URL field (as URL field is non required)

That is incorrect. It's actually a good thing that this is happening. This issue is to allow a default link text value without including a default URL value for a field. When the field is in use, it should still enforce the presence of a URL value if the link text is not empty.

So please, let's not bring again the form display behavior that is in core, which is correct, and it is not part of the issue we are dealing with here.

Here is the patch for 10.3.x, exactly the same as the fix committed in 10.4.x.

juanolalla → created an issue.

Thank you all, this has been committed to 2.0.x.

Thank you all, this has been solved in 2.0.1 →

Thank you all, this has been solved in 2.0.1 →

Previous attached file patch was wrong because the MR was created to branch 3.0.x by default.

Solved, I've opened a Merge Request. There is a failure in the pipeline regarding phpUnit which doesn't seem to be anything related to the changes.

Attaching file patch as well.

juanolalla → created an issue.

juanolalla → created an issue.

juanolalla → created an issue.

@waako Thank you so much! I'll take a look and review it as soon as I can

juanolalla → created an issue.

juanolalla → created an issue.

I started the Workbench port in https://github.com/backdrop-contrib/workbench

I've been trying the EncryptByProfile() method and it doesn't work, it always breaks:

In Url.php line 170: Route Required

Also, the comments of the methods are wrong, seems like an unfinished copy paste.

The last patch in #20 📌 Create a Bulk Encryption/Decryption Service Class to facilitate encryption of pre-existing data Needs review applies correctly in 2.0.x-dev, so it doesn't need a re-reroll

juanolalla → created an issue.

Fixed the referred coding standards: https://git.drupalcode.org/project/address_static_map/-/commit/4b66060de...

juanolalla → created an issue.

Drupal 10 compatibility implemented in 2.0.0

Committed to 2.0.x branch and available in 2.0.0 release.

Thank you all, this has been committed in a new major 3.0.0 release.

Sorry I couldn't answer before. If new permissions are added you're forced to make a new decision, whether to grant or not that permission. If you don't do anything, would mean your users will stop be allowing to do things that they could before, but now they can't because there is a new not granted permission. We tried to apply the most common logic case we thought should be the default.

@smustgrave, no it doesn't look like it have anything to do with it.

It looks like the current solution is failing accessibility tests because when duplicating the block form on a page, elements get the same id. I've verified that Drupal's way of preventing duplicates id by adding dashes and numbers only works with "edit-keys" and "edit-submit", not other specific ids.

So we would need to add a different random hash / number to the id string.

Uploading patch solving the issue.

juanolalla → created an issue.

It is not a duplicate of that issue.

The scope of the referenced issue you are asking about, is to discuss and find a solution to replace Drupal's prevention of id collision, on all forms. There is a tough debate going on about cache invalidation for form IDs, and no consensus has been reached yet, leaving to a situation where this has been stuck for 11 years!

I have opened this issue specifically for the Search module, because here is where we are finding let's say 99% of the collisions, due to having a search block in every page is so common. So the goal of this issue is to provide a simple solution: change the ID just in this form. We have been forced to do that in alter hooks for 10+ years, so let's fix this now with specific IDs for the form Search module provides, while we keep the other issue open to continue discussing (it's been almost 1 year without movement there), and maybe one day we have a better cache solution or ID prevention globally.

juanolalla → created an issue.

Thank you, I respect your decision.

I've released a module with this function so everyone can benefit of this improvement: https://www.drupal.org/project/twig_block_by_id →

Any rendered fields should be update automatically when changed. If you develop a custom field plugin it needs to be supplied with correct cache metadata for the page to be refreshed.

Thank you, but it is not the case.

Tested it with Quick Edit. Also worked for me.

I think you have some issues with your site configuration. Try to reproduce them on fresh Drupal installation.

Thank you for testing that. It also worked for me locally sometimes, not always, and never in our development server for example. So something else could be messing around.

What is true is that using drupal_block() solves this when this problem appears, so we can't use drupal_entity(). So that makes this new function simplifying drupal_block() signature a need.

Switching from drupal_entity() to drupal_block() does not just solve the caching issue. If also makes the blocks not to take the right template and configuration, so the blocks are not styled properly. This is because drupal_block() does not use the configuration of the block by default, you would have to provide it in the second argument.

In the solution I'm providing, I'm not only simplifying drupal_block() use by allowing to identify the block by id and not plugin. I'm also solving this problem by loading the configuration of the block by default, allowing it to be overridden.

I think all these improvements are really needed to make drupal_block() better and more usable.

Thanks for taking the time to review this.

Content blocks don't have titles.

Sorry I meant a custom field. I was using a custom block type and there I found the problem.

As of text (body). It should be updated automatically. I've just checked it using anonymous account.

The workflow would be to edit it from the page you are viewing (quick edit) with an authenticated and authorized account and then saving. It goes back to the main page, but not showing the changes, needs a cache clear.

Maybe something else from my site configuration and implementation is messing around. Maybe we don't need an issue because it is not a specific issue of this module. But the problem we are facing is that drupal_block() works well with cache where drupal_entity() doesn't. They do different things, from example, with drupal_block() you just have the option "edit" with quick edit, while with drupal_entity() you would have more.

So we need drupal_block() to avoid this cache problem, drupal_entity() is not exactly the same and doesn't work. And drupal_block() as it is painful to use for that need of the plugin, which requires to find the resource in the the config directory. drupal_block_by_id() work just fine, it's easy and straightforward, and it's adding value to the project. We could contrib a new module just with that addition (I already implemented it), but I thought this improvement would belong better to this original project.

What do you think?

Thanks Chi. However, drupal_entity() does not process the block rendering the same as drupal_block(). As a result, drupal_entity() bypasses the cache contexts while drupal_block() does not.

Right now users are facing the following problem:

1. Create a custom block and add it to the Block Layout page and configure it, disabled.
2. Add the block to twig template using {{ drupal_entity('block', 'my_block_id', check_access=false) }}
3. Go to the page and see the block appearing there
4. Edit the block, change some text or the title for example, and save
5. You will not see the changes appearing in the page!
6. Clear cache. Now you can see the changes

This shouldn't be happening, and I don't know if that has a solution inside drupal_entity() processing, or this is part of the way it is designed. If this have a solution, we should open an issue for that. But if not, the user is forced to use drupal_block(), and so for, users have to deal with the difficulty that I described in this issue and motivated me to contribute a solution.

Solved in merge request adding drupal_block_by_id() function.

juanolalla → created an issue.

I've created an issue in the github repository of the library: https://github.com/JJJ/chosen/issues/47

Tests passing, this is ready for review

Updating to 10.1.x, that's why test was failing.

My first commit failed because it worked in my 10.1.x version but fails in the latest code, probably because of this ckeditor issue that has been fixed: https://github.com/ckeditor/ckeditor5/issues/11709

Committing an attempt to fix it, blindly because I don't know exactly how to update or rebuild my local ddev setup to test the latest dev branch changes (setting the ddev/ddev-selenium-standalone-chrome addon which needs core-dev). I keep trying to figure that out.

I have expanded the FunctionaJavascript/StyleTest.php coverage to test styles for ul, ol and table elements (https://git.drupalcode.org/project/drupal/-/merge_requests/4462/diffs?co...).

It's not possible to apply a style to a div element is not working yet as far as I know.

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

I'm reopening the issue because despite the previous solution was considered not applicable, the problem is still there.

I think this is a pretty common use case, where you want to use the module and start by using the default css that comes with it (in tablesortertheme.css), like the header sorter images (arrows) and the rest of base theming. You could then adjust that with your own css in the theme.

Right now this is not possible, unless you select green or blue, and then you would need to override the css to remove that blue or green extra theming.

I understand that the first option was intended to be just the "System Default" theme, which means no specific css at all; and so for, adding tablesortertheme.css to that could be considered inappropriate and alter the styling of other sites with existing custom css without that file.

So I came with a different and simple solution from this missing use case: to create a new option to select "Tablesorter's Default" theme, so that way a user can count on the base css from tablesortertheme.css, without having to choose unwanted blue or green theme. Attaching the patch.

Opened a new merge request targeting 2.0.x branch

Changing version to the latest 2.0.x

@douggreen please see #78 🐛 The highlighting of the active menu-link does not respect query strings and fragment identifiers Needs review above, where I explained the problem I found with tests (tldr: wrong tests, js tests don't exist yet for this in core)

Thank you all, this has been included in the new released 2.0.0 version.

I like your solution, thank you very much. It works as expected.

Opened a merge request implementing proposed solution.

juanolalla → created an issue.

Here is a patch implementing the ability to hush specific upload extensions we know are safe in our site, like "html" would be in the use case provided as example in this issue description.

So now the form settings provides a textarea for the list of exceptions to hush, with a reason. This is similar to "Hushes" in "Dangerous tags in content exclude list". Those extensions will be hushed when the check runs.

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

juanolalla → created an issue.

Patch #79 wasn't working, the is-active stopped appearing (and tests failed). JSON.stringify() was adding double quotes, so we ended up having double quotes within double quotes in the queryMatchSelector string variable.

Fixed in this patch #80. Adding interdiff.

I'm uploading a new patch with just the changes to active-link.js. I think this is going to pass tests, and I'll explain why.

I'm removing the changes to ActiveLinkResponseFilterTest and UrlTest. They fail because there is no related implementation on php, which is what they are testing. They are testing the logic in ActiveLinkResponseFilter, that hasn't change at all. So it make no sense to change the testing logic without changing the code being tested.

So, as we have seen from the beginning, there are different places where the logic that states that a URL with query parameters is not the same as without or different query parameters in terms of marking links as "active", has been implemented, on the backend code, and with JavaScript. We are fixing this just in the JavaScript layer, and with that change, tests don't fail. That means that there is no JavaScript tests in core right now checking that the links shouldn't have the 'is-active' class when not matching query parameters.

So, to do this right, in my opinion, we should discuss if we are agree that the current logic is not right: all same url with different query parameters combinations (with the same language), should be treated as the same in terms of marking is as active.

Then, if we agree on that and this issue makes sense, we should create JavaScript testing cases for the new logic (there isn't any for the current logic at this level).

And then, we should rewrite the PHP code as well, and so for the tests managing that. Why do we have both PHP and JavaScript implementation. I would love to know.

In #54 patch I just re-rolled the merge request, I don't know who/when/why that logic changed in ActiveLinkResponseFilter.php at some point. As long as ActiveLinkResponseFilterTest.php is testing the logic well and the patch passes tests we are fine.

#73 patch is failing to apply. Does it target 10.1.x?

Fixed unit test failure from #39 with new construct() parameters.

I've re-rolled #27, which was the last working patch.