Merged for inclusion in the 1.2 release.
Verified internally. Will be merged for inclusion in the 1.2 release.
Merged for inclusion in the 1.2 release.
I've uploaded a new branch, 3511525-disable-reusable-blocks--parameter, that supersedes the previous one with an alternative direction of implementing the config for enabling reusable blocks as a service parameter instead of config value. I've redirected to this direction instead for the following reasons:
- A service parameter obscures the functionality more effectively than a new setting in the configuration form requiring continued usage of the functionality to be very intentional.
- Using a service parameter that's not heavily exposed also makes the process easier for phasing out the functionality altogether in a future release.
This new service parameter and related checks throughout the codebase default to disabling reusable blocks unless a site explicitly opts into enabling them through the use of the service parameter. This will make it easier to move toward removal altogether as part of 📌 Remove Reusable Block Support Active .
Included in this branch are the following changes:
Assuming the new enable_reusable_blocks service parameter is not set or is explicitly set to FALSE,
- Reusable block derivatives will not load in order do address the originally reported performance issue
- The following routes and related pages are no longer accessible or shown in menus:
/admin/structure/block/patternkit/patternkit_block/{patternkit_block}/patternkit_block/{patternkit_block}/edit/patternkit_block/{patternkit_block}/delete/patternkit_block/add/patternkit_block/add/{pattern_id}
- Existing reusable blocks originally created through the layout builder interface and placed in layouts should continue to render as expected
- Existing reusable blocks originally created through the Block Structure > Patternkit Library interface and placed in layouts will not render
- A status warning has been added to the requirements page if reusable blocks existing within the database
If the new service parameter is enabled, existing functionality should be maintained, but the status warning added to the requirements page will still be reported.
The new service parameter by be enabled on a site by adding the following to the site's services.yml file, typically found in sites/default/services.yml:
parameters:
patternkit.config:
enable_reusable_blocks: true
slucero → changed the visibility of the branch 3511525-disable-reusable-blocks to hidden.
Merged for inclusion in the 1.2 release.
This has been reviewed an tested internally. It is now approved for merging.
slucero → changed the visibility of the branch 3516087-update-json-editor to active.
slucero → changed the visibility of the branch 3516087-update-json-editor to hidden.
I've now finished addressing most of the code review feedback an pushed up several improvements to the MR that should make it significantly more robust and user-friendly in the case of a failure.
The change here looks good to me. We'll just do some light regression testing to make sure token replacement is still working as expected, and then we can get this merged in.
I've pushed up some work here to build on top of the patch from #6 and #7. My primary concerns with that implementation was a number of assumptions that are potentially site-specific:
- The selected media entity has a field named field_media_image
- This is the default name for an image media type, so the assumption isn't terribly risky
- The target pattern has several additional fields at the same level for population:
- alt
- width
- height
- These additional fields are not represented on the committed image pattern and would need to be added for testing and validation.
In the new commit I've added, the field name is addressed by looking it up from the media entity's field configuration. Regarding the assumed schema field names, I've built on the existing logic looking for the src attribute and allowed it to add the values for the additional fields and fail silently otherwise.
Since this only affects the image url solution, and not the media token solution that is now recommended, I'm not too concerned about adding additional testing and refactoring the example patterns to automate tests for this.
Merged for inclusion in the 1.2 release.
The work for this looks good, so I'm going to go ahead and merge it for inclusion in the 1.2 release. Thank you for the contribution!
I've pushed up some work that adds a new configuration option to disable loading these derivatives and further disables the implementation of reusable blocks altogether. To help move toward disabling support for these altogether, the option is currently getting set to FALSE by default and outputs a warning during the update hook if there are any reusable blocks present in the system.
Remaining work includes:
- Determine a migration path and implementation plan for sites currently using reusable blocks
- Add automated test coverage for each option
- General functional testing to make sure everything continues to work as intended
Merged for v1.2.
Reviewed and tested internally. Merging for inclusion in the 1.2 release.
Revert previous update based on documentation here with automatic service alias.
Correct typo in original service reference for decorator pattern.
Merged for inclusion in the next release.
Merged for inclusion in the 9.1.1 bug fix release.
@krisahil, good catch and thank you for opening up the MR to address it already! That looks good to me. Let's go ahead and get it merged in and I'll cut a new 1.0.1 release so it's available for anyone who hasn't started the process to upgrade to 1.0 yet.
Merged for inclusion in the 1.0 release.
Adding testing instructions to the summary.
slucero → changed the visibility of the branch 3468609-patternkit-1.0-release to active.
Merged for inclusion in the 1.0. release.
I was able to both reproduce this and confirm the fix. Good find and thanks for the fix!
I've added some tests to the MR for covering the new code changes, and everything looks good to me for moving forward with it. Unless I hear otherwise from anybody I'll get this merged in a couple of days.
Merged for inclusion in the 1.0 release.
This has been resolved in more recent releases.
Closing as no longer relevant.
Merged for inclusion in the 1.0 release.
Merged for inclusion in the 1.0 release: 🌱 Patternkit 1.0 Release Plan Active
Merged for inclusion in the 1.0 release: 🌱 Patternkit 1.0 Release Plan Active
Merged for inclusion in the 1.0 release: 🌱 Patternkit 1.0 Release Plan Active
Merged for inclusion in the 1.0 release: 🌱 Patternkit 1.0 Release Plan Active
Reviewed and approved internally.
Merged for inclusion in the 1.0 release.
Tested and confirmed internally. I'll get this merged in momentarily.
After further testing, I've uncovered that the existing behavior (the currently proposed default) is not terribly helpful for several reasons:
- When patterns are used in the default layout for a content type, the cache tags present for it don't matter as much for invalidation during pattern updates since the related updates to the default layout invalidate a tag like
config:core.entity_view_display.node.page.defaultwhich is present on every page of that content type using that display mode regardless whether it is overridden. Due to this, every page of that content type will be invalidated regardless. - When used on overridden layouts, including the invalidation of the
patternkit_pattern:<id>cache tags will result in all pages containing this pattern being invalidated when a single instance of the pattern is updated. These other blocks won't show any changes, however, since they still point to the prior revision of the pattern entity. The same behavior is true forpattern:<asset_id>cache tags which also get invalidated on update with this setting enabled.
Based on these findings, I'll be updating the default value for this new setting to FALSE which will represent a change in behavior for most sites already. This change should be for the better, however, by bringing cache invalidation more in line with visible changes to the content.