Account created on 17 January 2007, about 18 years ago
#

Merge Requests

More

Recent comments

🇺🇸United States jastraat

Could you provide steps for recreating this issue?

🇺🇸United States jastraat

The code changes look good; would like to see a review from someone actually using layout library though -

🇺🇸United States jastraat

I created an MR with my suggestion.

If this looks like a good approach, I could add the same logic in the unpublishing actions.

🇺🇸United States jastraat

I rebased on Drupal 11 and moved the new entity update hook to the hook class with the rest. I'd like to throw this back out for review -

🇺🇸United States jastraat

One big disadvantage of the current approach is that the original draft revision message is replaced with the generic 'Bulk operation publish revision' log message and you lose the information about what author originally made the actual edit.

I'd definitely advocate for retaining the information about who authored the changes and why.

🇺🇸United States jastraat

Just to confirm, this is still an issue with the latest release of Drupal 11.

🇺🇸United States jastraat

This works as designed; the blocks only really exist within the layouts that reference them.

If you feel otherwise, please open a "feature request" instead of a bug.

🇺🇸United States jastraat

I created a custom field formatter for the "type" base field on section library template.

This field formatter can then be used in the default section library view, so I updated the default view config.

I did not add an update hook for this since not all sites may want to use the new field formatter in their existing views.

🇺🇸United States jastraat

Looks good; created a followup issue to address the code quality issues.

🇺🇸United States jastraat

This looks great! Created a followup issue to address the code quality issues.

🇺🇸United States jastraat

For right now, running GitLab CI only in Drupal 10. Once we add support for Drupal 11, we should remove the following line from the gitlab file:
OPT_IN_TEST_CURRENT: 0

🇺🇸United States jastraat

jastraat created an issue.

🇺🇸United States jastraat

Thank you for that level of debugging and detail!

Does the attached MR address your issue?

🇺🇸United States jastraat

Thank you! Merging the 2.x one since that is the branch actively in development.

🇺🇸United States jastraat

Reducing to normal since this relates to an interaction with custom code.

I will point out that paragraph cloning was broken prior to 🐛 Error after importing section/template that contains blocks with paragraphs without available source content Active for paragraphs in inline blocks where the inline block was saved as part of a layout before creating a section template and then the original source block was deleted.

It looks like the hook is targeting a media entity. How does that interact with the paragraph and block entities? Is there a media reference field on the paragraph?

🇺🇸United States jastraat

Added a few checks to really make sure there's a label string.

🇺🇸United States jastraat

I don't think strtolower itself is deprecated: https://www.php.net/manual/en/function.strtolower.php

Did you recently update to the 2.0 release? Did you run update.php? (drush updb)

🇺🇸United States jastraat

That sounds awesome!

As a followup, we were able to get the new branch of editoria11y to not time out by reworking the mega menu JS (and probably improved site performance along the way!)

🇺🇸United States jastraat

Unfortunately since templates can have additional unknown fields, this approach may be really confusing for editors unless there was some additional magic to have editors first pick whether or not they were creating a new template or updating an old and then as a 2nd step if updating, populate the entity form with the existing section template field values. I'm just not sure how feasible this approach is when there could be additional custom fields on the entity.

🇺🇸United States jastraat

I love this idea but new features are only being added in the 2.x branch and that branch has changed enormously since this MR was opened. If I get time, I'm going to try creating a new branch/MR based on your work that is compatible with the current 2.x branch. (I already started working on this; there are additional considerations since we only want folks to be able to update templates they have access to edit, sections and templates are now using content entity forms/fields and those two types were merged into the same add form to reduce duplication.

🇺🇸United States jastraat

I'm marking this as reviewed since I didn't write the original patch and tested it. This has also been implemented on numerous other modules for the same reason.

🇺🇸United States jastraat

Could you check to see if this issue still occurs with the new 3.0 release?

🇺🇸United States jastraat

I'm actually no longer seeing an issue with spaces in file names with the latest version of spatie (from 📌 Update module to be compatible with latest spatie/pdf-to-image library and manage the library with composer Active )

🇺🇸United States jastraat

I looked into the difficulty of having different title text for the contextual link depending on if it was being shown or hidden. Having dynamic text can be done by extending ContextualLinkDefault and overriding the getTitle() method, however any kind of dynamic title will be complicated by the fact that contextual links are cached in local storage.

The current MR's tests are passing. If having different text for the contextual links is the only hold up, could we consider an alternative label that doesn't change?

Marking as needs review in hopes of getting this merged -

🇺🇸United States jastraat

Nice solution! This worked well in my testing.
Would someone else like to review the MR before I merge?

🇺🇸United States jastraat

Could you try the MR in https://www.drupal.org/project/section_library/issues/3512185 🐛 Existing test are failing after new section library module update Active ?

🇺🇸United States jastraat

I think the solution is to remove all image style config from the install config; we cannot assume that an image style still exists (even if it was included in the default installation of the core image module)

🇺🇸United States jastraat

Many of the fixes from this patch have now been merged in other separate issues; marking as outdated.

🇺🇸United States jastraat

The image rendering was fixed in https://www.drupal.org/project/pdf_preview_image/issues/2954384 🐛 Poor image quality Needs review

While this removes the need for the wrapper library from imagemagick, it's just a trade of dependencies. https://www.drupal.org/project/pdf_preview_image/issues/3511732 📌 Update module to be compatible with latest spatie/pdf-to-image library and manage the library with composer Active approaches this in a different way by including the required wrapper library in the module's composer file.

🇺🇸United States jastraat

If using Drupal core's built in settings for replacing spaces and special characters in file names, this is no longer an issue.

🇺🇸United States jastraat

While this could make sense if pdf preview image is only providing a default in the case that a user doesn't upload their own thumbnail, the patch hard codes a field name and in it current format would also prevent a new thumbnail from being generated if the PDF was replaced.

🇺🇸United States jastraat

In addition to the automatically generated MR, I updated the format of the dependencies to match the recommended format for Drupal (namespaced in the format {project}:{module}).

Given the warning in the issue description:

"Warning: The 'project-update-bot-only' branch will always be overwritten. Do not work in that branch!"

I created https://git.drupalcode.org/project/pdf_preview_image/-/merge_requests/11 and tested this in a Drupal 11 site successfully.

🇺🇸United States jastraat

Made an MR from the patch. Confirmed the error prior to the patch and then confirmed that the patch/MR fixed it.

🇺🇸United States jastraat

This is really cool functionality!
I did get an error when I first tried to preview so I made a tweak.
I think this may also require a cache clear from a post update hook.

🇺🇸United States jastraat

Tested with and without the patch, and the image quality is noticeably improved with this change. Created an MR that reflects the patch.

🇺🇸United States jastraat

jastraat made their first commit to this issue’s fork.

🇺🇸United States jastraat

I created an MR from the patch and added some code cleanup so that the new code quality tests pass.

🇺🇸United States jastraat

jastraat changed the visibility of the branch 3192704-create-directory to hidden.

🇺🇸United States jastraat

jastraat made their first commit to this issue’s fork.

🇺🇸United States jastraat

The preview generated without an issue for the MR.

🇺🇸United States jastraat

jastraat made their first commit to this issue’s fork.

🇺🇸United States jastraat

I'd like to suggest reopening this issue since the previously linked issue is now needs work and labeled a feature request.

🇺🇸United States jastraat

Updated the MR to include logging without injecting the logger. There were pre-existing calls to the logger service for exceptions so this was consistent with the existing code. Also updated the test for the latest Drupal.

🇺🇸United States jastraat

jastraat made their first commit to this issue’s fork.

Production build 0.71.5 2024