Account created on 11 July 2012, almost 12 years ago
#

Merge Requests

Recent comments

πŸ‡ΊπŸ‡ΈUnited States joshf

@sandzel it looks like you're done working on this? Moving to needs review.

πŸ‡ΊπŸ‡ΈUnited States joshf

This fixes an error that popped up in our automated testing.

πŸ‡ΊπŸ‡ΈUnited States joshf

Hi @mjgruta, thanks; your latest change fixes the fatals I was getting with the previous one.

πŸ‡ΊπŸ‡ΈUnited States joshf

This patch fixes the fatals caused by the previous change.

πŸ‡ΊπŸ‡ΈUnited States joshf

joshf β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States joshf

It's actually the group menu type name in the parentheses; updated the steps to match.

πŸ‡ΊπŸ‡ΈUnited States joshf

Attached a patch that updates the documentation with my steps above.

πŸ‡ΊπŸ‡ΈUnited States joshf

The missing steps in the documentation are:
- Browse to a group page
- Click the "All Entities" tab
- Click the "+ Add new entity" button
- Click "Group Menu ({organization-name})"
- Fill in a Title
- Click the "Save" button

πŸ‡ΊπŸ‡ΈUnited States joshf

#42b did not seem to fix the problem in our case. We went with this as a temporary workaround:

.ck-content ol[type='A'] {
  list-style-type: upper-latin;
}

...and so on down through the list hierarchy.

πŸ‡ΊπŸ‡ΈUnited States joshf

@cinglefield1 the patch won't install via composer on Drupal 10 unless you use something like drupal-lenient β†’ . The reason for this is that composer checks the module's composer.json for compatibility before applying any patches. Since the unpatched module doesn't have D10 compatibility listed, it rejects the module entirely.

I can reproduce the error you're getting with [node:nid]. It's not a regression caused by this patch but it looks like the patch may not fix all PHP8.1 compatibility problems. Back to Needs Work.

πŸ‡ΊπŸ‡ΈUnited States joshf

I took a different approach for this. Instead of a configuration form, I opted to use the default file scheme as suggested by Damien in the D7 version of this issue. β†’ . This works great for us with our s3 setup, but there is one weird caveat.

In 2715669 β†’ , the thumbnail URL was changed to relative to keep content portable between hosts. Unfortunately this doesn't work with s3 because the normal operation is that the image is on a different host from Drupal. I've tried to work around this by first checking if the hostname of the preview image is the same as the Drupal hostname. If it is, the existing behavior is retained and the image is output as relative. If the hostnames differ, the full path including original host is used.

The only time I can see this being an issue is if someone is staging their content on a server that uses a different s3 server than production, which sounds super complicated. In that situation, preview images are already broken, so this doesn't break anything new but does fix s3 preview images for those of us who aren't staging their content.

I'm leaving #2's patch visible since it's a completely different way of going about it that might work better for some.

πŸ‡ΊπŸ‡ΈUnited States joshf

The patch in #5 solved the problem for me; thanks!

It does need the test to be completely rewritten, as the current test only checks to see whether an item can be rendered more than 20 times with the same context and fails if it can (which is just completely wrong). The correct test won't be trivial to write because I think you'll need to provide some mock classes β†’ to set up the correct behavior. The patch also needs code quality fixes.

#11 is a very limited workaround, not a fix, and should not be considered for merging.

#15 is simply incomplete; the way it's written no behavior is changed. It might work if there was a call made to ::setRenderPath(), but there isn't. I'm hiding it because it's just not a solution and will only confuse things.

@wylbur this issue is specifically for the entity_embed module; the Drupal core patch you referenced will not fix this issue. Further, I don't think patching this module will fix any problems you're having with paragraphs.

πŸ‡ΊπŸ‡ΈUnited States joshf

Attached a new patch that will apply to HEAD.

Again to clarify:

#31 applies cleanly to the distributed version of 8.x-1.0-beta5 πŸ“Œ Automated Drupal 10 compatibility fixes RTBC , and is suitable for use in composer patches.

#38 applies to HEAD, and should be suitable to merge.

Thanks!

πŸ‡ΊπŸ‡ΈUnited States joshf

@fehin @cinglefield1 you'll need to apply the patch attached to this issue in order for the module to work on Drupal 10. If you're using composer, I suggest composer patches as a way to have composer automatically apply patches for you.

πŸ‡ΊπŸ‡ΈUnited States joshf

joshf β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States joshf

We use this module on NSF and have an interest in keeping it up to date. I haven't maintained a contrib module before but this one is simple and looks like a good starting point. My mentor and coworker @dmundra β†’ is going to co-maintain this with me, the idea being we handle this as an organization and not just individuals. Thanks!

πŸ‡ΊπŸ‡ΈUnited States joshf

We've been using this successfully; thanks for the patch!

πŸ‡ΊπŸ‡ΈUnited States joshf

#2 applied cleanly for me and doesn't seem to cause any problems. Thanks for the patch!

πŸ‡ΊπŸ‡ΈUnited States joshf

Same as before but a real file this time :P

πŸ‡ΊπŸ‡ΈUnited States joshf

Attached is a patch that will apply cleanly to the 8.x-1.3 version packaged by d.o.

πŸ‡ΊπŸ‡ΈUnited States joshf

Attached a patch that will apply cleanly to the d.o. packaged version of 8.x-1.0-beta5. This can be used to patch the module via composer. Be aware that if you want to update to D10 via composer you'll need to use one of several available workarounds β†’ to deal with the fact that composer cannot patch in a dependency change.

πŸ‡ΊπŸ‡ΈUnited States joshf

I experienced this error as well. I wasn't able to fix it, but I was able to identify that a ComputedField object is making it into EntityFieldManager::fieldDefinitions when it should actually be a ComputedFieldDefinition object.

πŸ‡ΊπŸ‡ΈUnited States joshf

No actually the steps are not identical. I wasn't testing with a clean D9.5 install. Also, scheduler_content_moderation_integration was never installed at the lower version. It was installed at beta1 alongside the scheduler upgrade. So there's probably some order of operations that differs.

I was able to work around the problem by removing scheduler_content_moderation_integration from the site entirely.

πŸ‡ΊπŸ‡ΈUnited States joshf

We're having the same issue. As far as I can tell, this patch doesn't make any difference. We're still getting dozens/hundreds of entity_form_display configs being created after enabling scheduler_content_moderation_integration and updating scheduler to 2.0-rc8.

πŸ‡ΊπŸ‡ΈUnited States joshf

we should see if we can module weight system not have explicit numbers but rather be configured on their dependencies instead

Is that work being done somewhere? If so, that work should block this, right? If not, this work definitely shouldn't be merged, right? Until that refactor is done, module_set_weight() is critical functionality with no replacement.

Right? I'm not missing anything?

πŸ‡ΊπŸ‡ΈUnited States joshf

This patch changes the state to use visible instead of enabled.

πŸ‡ΊπŸ‡ΈUnited States joshf

This patch fixes the #states value of the new form element.

πŸ‡ΊπŸ‡ΈUnited States joshf

Attached patch re-implements the change from #2372781.

πŸ‡ΊπŸ‡ΈUnited States joshf

My feeling is that the correct fix for this is a hook_uninstall() which restores extlink.settings extlink_alert_text to a string instead of an array. I'm not 100% sure what the right way is to go about this, though.

My instinct is is to just take the 'value' out of the array and call it a day, but that value was set with the intention of using the given text format, which might not be what the site admins want.

Another option would be to store the last string value at the time it is changed to an array, and restore that value. But that could be completely different from the current value.

πŸ‡ΊπŸ‡ΈUnited States joshf

I wasn't clear what the previous comments meant by "exporting the empty conditions object". What I did was manually edit the config yml to match what the second environment was outputting. Then after applying the patch the exports matched. +1 RTBC for me; thanks for the patch!

Also hi @jds1 <3

Production build 0.69.0 2024