🇺🇸United States @bkosborne

I discussed with partyka and we think this is actually not a problem and the issue can be closed. At the very least, this should have more clearly defined description of what the problem is.

It sounds like what really is the problem is that the act of deleting and re-importing nodes created a ton of stuff in the trash with no clear way to purge the trash. Trash should have a purge all functionality or a multi-select for purging as was suggested in the issue summary.

But the current behavior I think makes sense, as I outlined in #6. An alternative behavior would be if Feeds didn't re-import items that were already trashed, but I think that's even more confusing.

Hi Rajab,

I'm still a bit confused about the distinction.

What makes you say that Bookmarks are visually marked anchors on the final front end? The documentation for the CKEditor plugin doesn't mention that.

The plugin implements the bookmarks in the same way that Anchor Link does. It creates a <a id="name"></a> element. That wouldn't be rendered on the front-end in either plugin unless you added specific styling for it.

Damn, my memory is failing me. I created that issue 8 days ago!?!

I thought this to, but the action plugin this module provides is currently applicable to nodes, which all have revision support.

So I think we can close this, or perhaps we change this issue to make the module work with other entity types? Though, at the moment I have no need for that, and prefer to keep the module as simple an narrow in scope as possible. There is Views Bulk Edit → that works with all entity types and has many more features that people could use as well.

bkosborne → created an issue.

Confirmed, it works well with layout builder. Thank you!

Ready for review!

Thank you!

Agreed we should be consistent! I converted all the local vars to snake case. I do use camelCase for everything most of the time out of habit, but I think the module already was using snake_case, so I switched to using that.

Thanks for the quick reviews!

Okay, ready for review!

I'm pretty sure if no version is present then Drupal calculates a hash of the file contents and uses that as the version on cache clear. Are you sure that's not happening? I know it works that way for asset aggregates.

If it works as I think it does, then I'd rather leave it as is. We're likely to forget incrementing that version on future updates.

Fixed! Once this is merged, I can start work on the other two issues I created.

Thanks for the review! I addressed the feedback and pushed.

Yea, I think we're good at just requiring Drupal 10, which requires PHP 8.1 already. I don't think I used any language features beyond 8.1.

Okay, MR added and ready for review.

Maybe! It may be a bit tricky to pull the logic out of the main module.

bkosborne → created an issue.

bkosborne → created an issue.

bkosborne → created an issue.

bkosborne → created an issue. See original summary → .

Duplicate of 📌 Require Views Bulk Operations in composer.json Active (I know this one was created first, but more activity in the other)

I cannot reproduce either. This issue is 3 years old so likely no longer relevant.

Not enough information here. Since the issue is 3 years old, going to close it out.

The module's README has this:

(Optional) Define a "bulk_edit" form mode for the entity bundles
that you wish to be able to bulk edit. Under the "Form modes" page
at /admin/structure/display-modes/form you can add a new form
mode. If it uses the machine name "bulk_edit", the form for the
"Modify field values" action will use that form mode to determine
what fields are available for bulk editing.

I spent a few days debugging the issue and determined that the Trash module was causing mayhem with Feeds.

What mayhem though? I think this needs more specific steps to reproduce. What exactly was the issue? (I know this is a couple months old and may not be fresh on your mind anymore!)

Here's what I did:

1. Tested on a site with Feeds and Trash enabled
2. Created a feed type to import from an RSS feed on some other website to a content type that had Trash enabled
3. Ran the importer which imported a bunch of nodes
4. Deleted one of the nodes (moved it to trash)
5. Re-ran the importer
6. Observed that the importer imported the trashed node again

This is what I expected would happen. When feeds went to check if the node already existed or not, it found that it did NOT exist because it was in the trash and the Trash module is "hiding" it from anything that goes looking for it.

I tested this again with standard profile on 10.4, 11.1, and 11.3 and I can no longer produce it. The first hit to a 404 path, X-Drupal-Dynamic-Cache gets UNCACHEABLE (poor cacheability), but if I make a 2nd request to the same 404 path or some other 404 path, I now get `HIT`.

This still confuses me, but it's better behavior than what I saw before (or thought I saw before).

Note I was testing just with standard profile, but I uninstalled Internal Page Cache so it didn't interfere with anything.

I'm tempted to close this, but catch did say there's some legit reason for some of this.

Sure, this was the fix for this module.

Yea this was missed.

We have only one feature that uses this, which is the client-side authentication method for gateway authentication. I imagine most sites don't use this, so we should consider not installing the module for everyone.

Updated the proposed resolution...

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

Created a new branch from 3.x.

bkosborne → changed the visibility of the branch 3275551-when-bulk-adding to hidden.

bkosborne → changed the visibility of the branch 3.x to hidden.

bkosborne → changed the visibility of the branch 2.x to hidden.

bkosborne → created an issue.

Okay, trash module released 3.0.17 which includes my fix. So I think our tests here should pass now. I'll re-run them.

This would only apply to 4xx responses, because 5xx responses are already excluded from code above, which checks first if the original response is considered cacheable. It won't be with a 5xx code.

I'll take a look at adding this option this week...

Content editor wants to do something/anything with the original /policies page. When the content editor goes to /policies to **edit** that page, they are now redirected and probably quite confused. Even if the content editor knows what's going on, they would now have to find the node in the content listing /admin/content and click "Edit" to be able to edit the content.

This happens to our editors frequently. We don't use a patch from this issue, but our editors and team know that you can do the workaround of creating a redirect from an existing path by using the system path as the source. E.g., instead of creating the redirect from /policies -> new page, you create it from /node/123 -> new page. The end result is the same level of confusion. The only way to edit the original page is to find it in the content dashboard.

bkosborne → created an issue.

bkosborne → created an issue.

+1 to this!

Is it possible there's some other module interfering? This module's code is designed to not do anything if the "override" checkbox is not checked.

I'm not able to reproduce this. Do you have specific steps to reproduce on a clean install?

bkosborne → created an issue.

Agreed. I fixed it in the MR for that other issue, so I'll mark this as a duplicate.

Okay, the blocker from Trash is in. Just need to wait for its next release, then these tests should pas.

bkosborne → created an issue.

test failures are unrelated

Okay, test is failing because I was working with this patch for Trash 🐛 Trash's hook_entity_access should account for the 'view label' operation Active . Without that patch, trashed entities always show with a "- Restricted access -" label, even if the user has access to view trashed entities. It's a super simple fix there, so hopefully I can push that one through. But I'll postpone this issue on that.

Note there's also some phpcs failure but I don't think it's related to these changes.

Are you able to reproduce this on a clean Drupal install with only this module enabled?

That's an interesting solution, but adds a good amount of complexity around generating the new directory structure. I don't really understand why it's important to have the same filename? Why not just append a unique number to the end of the file, delete the original, then add a redirect from the original? That's much less complicated. The client is that insistent that the filename be unchanged?

bkosborne → created an issue.

Okay, I'll work on this. I also polled some coworkers about this and they all agreed that it makes sense to show the trashed sources and to indicate they are in the trash.

Thank you!

Thank you!

bkosborne → created an issue.

I combined the fix for this with 🐛 Exception thrown on usage page if a layout builder block has usage but its parent entity was deleted Active which has an MR.

I found 🐛 Entities used by content blocks should not use label or status of layout builder node unless they are "inline" (non-reusable) Active while testing this. It's much easier for me to combine the fixes in one merge request (which I'll do here). I can split them out if needed, but in that case I'd rather get this one committed first. I did lots of refactoring of the test which is hard to unwind.

bkosborne → created an issue.

bkosborne → created an issue.

That was quick, thanks! I was planning on working on the test today, thanks for taking care of that.

bkosborne → created an issue.

bkosborne → created an issue.

There's lots of conflicting information on how to properly handle this. Here's some resources:

• OWASP's CSV Injection page
• The Absurdly Underestimated Dangers of CSV Injection
• Vulnerability report of old PHPMyAdmin version

They all suggest slightly different things.

I created an MR that detects if a string starts with certain problematic characters and if so, prefixes it with a single quote. I'm not 100% that's enough. John suggested doing it based on the presence of any non-alphabetical first character, but I think this is likely fine and is more inclusive of non-English speaking languages that are much more likely to begin with different types of characters.

bkosborne → created an issue. See original summary → .

Hmm I'm not sure about this change. We use the tooltip mode (we don't link to the tooltip term pages, we just use the tooltip abbreviations). With this change, if the term first appears in some link on the page, it doesn't get tooltip'ed anymore. I think it should. The link is not going to the term page in our case - it goes to some other page on the site. I think that's what #6 mentioned, but I'm not sure this was addressed? Maybe this new functionality should be limited to only configurations that use "links" or "tooltip_links" setups?

Looks like the selector was already updated to use a "contains" query in the past few releases, so all that's needed here is to add the "glossify-exclude" class to the default template for tooltips. I'll create an MR for that and see if existing tests pass.

bkosborne → created an issue.

This bit us too. I think a better approach for this element would be to render it as a fieldset, where the element name is the fieldset name, and then the individual date parts have visual labels for just the part above. That way the date part labels would not be so long and redundant:

Thank you!

bkosborne → created an issue.

bkosborne → created an issue.

Ah, sorry. The MR seems ... massive. It looks like it combined work done in a separate issue for translation support. There's been a lot of progress lately in Trash, so I think at the very least it needs to be updated. The "deleted" field in the update hook doesn't make sense, the module today already provides deleted field. If you install and uninstall the trash module without this MR, does the deleted field linger still? If so, that's when a new issue should be made.

daniel, can you create a new issue for that?

This is the problem I described in #19. I "fixed" this by using CSS to overwrite the size of the images.

Agreed on getting this in. I fixed the merge conflict in the branch. The CI failures are not from this merge request. The unit tests are passing.

This looks good to me. This just bit me again 2 years later. This description would have helped. Thanks.