bkosborne → created an issue.
Looks like this was already done in this commit which was released in 1.1.0.
bkosborne → changed the visibility of the branch project-update-bot-only to hidden.
Dup of 📌 Automated Drupal 11 compatibility fixes for bibcite_pubmed Needs review
You should click the "plain diff" link next to the merge request link (top of the issue on this page). It generates a git patch file. You should copy the contents of that into a plain text file in your project and include it with composer patches. Looks like it's already D11 compatible.
The thing holding up this issue is the lack of tests.
Merged 11.x in
Hiding patches so we can focus on the MR
Still NW for tests I guess, but I created an MR at least.
Agreed all around. This looks ready to go.
Looks good, thank you!
Hmm, the only place I see in the code where an account is switched is FeedsExecutable->processItem()
bkosborne → created an issue.
This works as expected and restores the original toolbar. RE: #32, I think it's best that be handled in a follow up so we can restore this critical functionality.
Pencil icon was still broken in the latest MR. I fixed. Will merge and cut a 3.1.0 release soon.
bkosborne → made their first commit to this issue’s fork.
Thank you!
A site's UUID wouldn't be ideal here because we also need to ensure the hash tags are unique per a site's environments. E.g., if the site is synced from prod to a test environment, it will have the same UUID but shouldn't share the same cache tag "namespace" with production.
Using hostname is a safer choice, but still not perfect. I imagine people are using Drush for Purge invalidations, likely via cron. Some people may not be setting the hostname when invoking Drush, so it will end up being "default" or something, so the tags wouldn't be invalidated correctly.
Not sure what other option there is though. Perhaps make the cache tag prefix configurable and let the site owner decide?
Hi erickbj, I wonder if you're using this code in production since you've introduced it? Has it worked out well for you?
This is closely related to ✨ Add optional Environment setting Needs review . In fact, the solution here should be bundled with that change IMO.
Yes please. We don't use much from the SDK, should be straight forward to decouple from it. I'll get started on this now. I also agree that just copying code from the SDK isn't wise. We can get even simpler.
bkosborne → made their first commit to this issue’s fork.
Just a note that if/when we implement this, we'll need a release note to indicate that the entire Cloudflare cache should be purged, because all the cache tags will have changed.
Patch from #8 appeared to have unrelated changes. I just updated the merge request to be compatible with latest from 7.x and hid the patch files.
Please, use MR updates for future updates, not patch files.
Nevermind, the latest patch breaks things. The linkit autocomplete isn't used at all with it. Needs some work.
looks good to me!
Okay, I refactored the logic here to be more precise in finding the correct title field to manipulate and to fix things so that if a title field cannot be found, the basic functionality of the selection handler (populating the uri input field with the clicked autocomplete value) still works.
Note that the logic for assigning $context variable is flawed as it doesn't scope correctly when there's no fieldset wrapping the link field (there won't be if the link field doesn't have a title field output).
const $context = $(event.target).closest('form,fieldset,tr');
But as far as I can tell, it's not that important and affects only people using LinkIt with CKE4. Given that's deprecated I think it's best not to touch it.
I marked 🐛 Linkit field doesn't work in nested Paragraph when Auto populate link text is enabled Active as a duplicate of this.
Maintainers, please add users "mahde" and "deulenko" to issue credits.
I'm confused about the patch in #4 as it is completely different from the patch in #3.
It sounds like the root of this problem is that the logic for finding the title field is flawed. There's other issues with this logic too that I discovered in 🐛 Field widget broken if there's multiple link fields, one without link text enabled Active . I created an MR in there to cover those issues as well as the issue here. I think this can be closed as a duplicate of that (I realize this issue was open first, but I'm not sure what's going on with the MR/patch here, and my issue covers additional issues. I'll carry over the issue credits to that one).
bkosborne → created an issue. See original summary → .
Note the LinkIt module provides a field widget for Link fields that does all of this and more. I can imagine LinkIt being absorbed into the core in the future.
This is old and seems unrelated to the module.
Agreed, looks good!
Agreed with this change, looks good.
greggles → credited bkosborne → .
Thanks. I'll commit this and release later this week.
Thanks! This is good to go. I did push another commit that explains why we're doing this too.
I put some more thought into this given your suggestion. But I've become more convinced that the current behavior is a bug and shouldn't be a choice someone can opt into. I think it's totally unexpected behavior to have a change to a node not update the most recent version of that node, but instead update the published version.
What I'd really prefer to do here is to fix this to load the latest revision, and wait and see if anyone has a need for the existing behavior. This module isn't widely used (yet) so I suspect there aren't any or many people using it with Content Moderation. Basically, I think it's a good idea to keep the module and config options as slim as possible while we can.
Ready for review.
bkosborne → created an issue.
Thanks!
Yes, adding Tugboat previews would be great. We use tugboat quite at bit at my work. If you create the issue I can work on it.
Ready for review.
Since the autocomplete widget and the multi-value select widget submit form values differently, I had to update our form submit handler to normalize the results, but I think it resulted in more readable code anyway.
Ah, nice find. Merging!
bkosborne → made their first commit to this issue’s fork.
Thanks!
eh, while I could do the change to select widget in a form alter, it's more complex that I'd like since we need to also load the term tree. Creating follow up to add it to the module...
bkosborne → created an issue.
Ready for review. Sorry, the changes look bigger than they are because I had to refactor how the form is constructed a bit. Now the code first gathers the list of all eligible term reference fields, then in a separate loop builds the form. This allowed me to collect the list of content types used by a field with the same name.
Agreed. I'll split that out into a separate issue. I may not even do it here. May be pretty easy with a form alter for our own use case.
bkosborne → created an issue.
Moving this issue to Media Entity File Redirect. I think that's the best place to solve this.
We came across this as well.
If the Media Entity File Redirect module (which I made) just modified the canonical path of media entities to be /download/[id], everything would just work. The reason the module doesn't do that is that the redirect route is optionally enabled per media type, and I don't think we can override the canonical link template per-bundle. It's the whole entity type. So instead, the module simply provides a route for the redirect path and provides LinkIt matcher and substitution plugins.
One possibility is to have LinkIt detect if Media Entity File Redirect is enabled, and if so, check if it's enabled on the entities type. This seems messy to me. I think it's best to avoid having one contrib module have feature detection of other contrib modules.
Another possibility is to fire an alter hook for the default value in the field widget? Media Entity File Redirect could then implement the hook and set the correct default value.
Without such a hook, the module would need to alter the field widget via media_entity_file_redirect_field_widget_single_element_linkit_form_alter, which may work. I'll investigate that.
Thank you, but we really need steps to reproduce here so I can be sure it's not some conflict with some other module.
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)