$this->cssCollectionOptimizer->deleteAll();
This is still in the theme installer.
Needs the patch converted to an MR, or just a fresh MR created removing that line. Not sure we need tests for this, we'd just be testing that we removed the code really - e.g. we'd need to get some CSS aggregates created on disk, install a theme, make sure they're still there. But also with 📌 Bring back the asset stale file threshold Active new aggregates wouldn't get deleted anyway, making this impossible to test. Given that, removing the needs tests tag.
Had another think about it and we might be able to do this as the follow-up: 📌 [12.x] Try to use property hooks to lazy instantiate entity keys Active .
(unable to install due to existing config)
Can the config be moved to optional? That ought to not complain.
Wondering if we want a follow-up for #28 / #29 to avoid loading the user entity at all, whether that's worth attempting will depend on 📌 Load user entity in Cookie AuthenticationProvider instead of using manual queries Needs work .
We could move the global CSS/JS into a dedicated library attached seperately.
That sounds like a good idea to avoid OOM for sites that have configured global CSS/JS. I thought I had a way around this when I commented on the MR but if that global css/js currently has to be added to the individual webform library definitions then I don't think there's an alternative to splitting it into its own thing.
A separate library for that would also help with asset aggregate hit rates (e.g. one library for all webforms, instead of unique one for each webform but with the same actual CSS/JS in it) - at least if I'm understanding what the differences are - not actually using the feature anywhere that I know of.
Had to update CurrentUserContextTest to create the anonymous user in the database and fix assertions - it's testing for completely the wrong behaviour. The change here makes the test 'fail' but only because it's causing the anonymous user to be available whereas it was previously not found.
We've been using div embeds for H5P types that supposedly only support iframe, while I can see the reasons, there are other ways to handle js/css interference.
The big problem with the iframe embeds is the interaction between the Drupal asset system and the H5P API - the H5P assets are being put into the asset API, then taken out again. If there was a way to make it more genuinely standalone that might be OK too, but it would be a significant change to the module.
Performance tests show a clear improvement. This means #38 is wrong, could be one of two reasons:
1. We were already getting field definitions in other places ten years ago.
2. We're getting field definitions now that we weren't previously.
But given this is a clear improvement, I think we should go ahead with what we originally planned.
📌 Make EntityTypeManager provide an entity factory Needs work is still there to defer things even later.
This is critical for PHP 8.5 support.
#28 might have been a red herring all that time ago, or have become one - while constructing an entity still gets field definitions, so does accessing any field.
I tested with Umami, with the 'disclaimer' block visibility set to anonymous users, and with page cache uninstalled, and after truncating dynamic_page_cache (could have uninstalled dynamic_page_cache_instead).
Drupal\block_content\Entity\BlockContent::isReusable() and Drupal\user\Entity\User::getRoles() both call Entity::get(). It's pretty likely that the current user context being in use means that at least one field will be accessed, so we might as well save loading the entity.
Total function call difference in xhprof is -200 but I'm hoping we'll also see a cache get or some queries disappear in performance tests.
I double checked brandfolder and there are only ~5 sites running the Drupal 11 compatible version of the module (and it also supports Drupal 10 so they may still be on Drupal 10 too), so I think we are definitely OK with the change record and issue. The getter has been around for ages and the bc policy excludes protected properties of non-base classes (and especially the values of those properties), can't think of a less intrusive way to make the change.
Side note: this is one of the first issues where it's affecting performance tests (intentionally) and I had essentially zero involvement at all, so I actually got to see test changes without any previous context or knowledge of the issue, it's nice to see how the coverage shows the relative improvements between different scenarios, see the query diff etc. - so much easier to understand the consequences. Also don't think this would have been obvious to find via profiling, it's really the database query that makes it obvious what's saved.
Committed/pushed to 11.x, thanks!
Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!
Committed/pushed to 11.x, thanks!
Committed/pushed to 10.6.x and cherry-picked to 10.5.x, thanks!
Went ahead and merged the MR, I've opened 🐛 H5P settings not always available in iframe Active for the remaining issue.
catch → created an issue.
@vensires ah I see - no I made that mistake the first time I tried to reproduce, but have been testing with the iframe since (against 10.5, but not against a completely clean install, which may explain why you're still seeing the drupal settings error while I'm not - it could be down to another asset ordering issue or even page timing).
I'll go ahead and commit here and open a follow-up at the same time.
Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!
Pushed a change to change the dependency from core/drupalSettings to core/drupal. per #25 The previous js was relying on those implicitly via using globals, now we rely on the actual core APIs we also need the dependencies.
Really the static library definitions should be moved to an h5p.libraries.yml file leaving only the dynamic definitions in the hook, might open another issue for that.
Moving this back to needs review for more testing
@vensires a behaviour change in an alpha release doesn't require a new major release, but if you'd like to discuss that please do so on the issue I opened specifically about it linked above rather than here.
Committed/pushed to 11.x and cherry-picked to 11.2.x/10.6.x/10.5.x, thanks!
Adding some related issues.
I think my main question is if an issue brings a UI up to 2.1 (or keeps it at 2.1), but a 2.2 issue is discovered with that same UI, then I don't think we should block on 2.2 issues but open separate issues to deal with those. I don't know if that's even a real concern though.
If/when we do 📌 Automated A11y tests in PHPUnit Active I would also want the option to set it to a lower level if we can get tests to pass, then open another issue to go to 2.2 and open issues to fix those issues so we can enforce it. We've done this for many similar changes like phpstan because the tests prevent the regressions even at a lower level, whereas no tests don't prevent any regressions at all.
Both of these are about how we get towards 2.2 rather than whether we should though, it makes sense to have that as the target.
This one is unfortunately still valid, the code has barely changed.
@jrockowitz yeah that could be a good option!
Storing the entire library definition in state could be heavy now because state is cached with a cache collector, but could use key/value directly instead of state - this is only used when building library definitions so doesn't really need to be chained fast/state as such. But also if it's only a list of which webforms have libraries that would definitely be small enough for state.
Either way would be simpler than adding the new keys to the config entities.
Adding an MR that silently truncates the key value name to 128 to see if any existing coverage fails.
Opened 🐛 The config key_store (entity queries) can't handle long key values Active .
If that issue isn't fixable in a way that helps here, another possible approach would be to add 'has_js' and 'has_css' keys to webform config, obviously without a UI, then populate them based on the js/css keys - these would only need boolean values so would then fit in the config entity key_store and be queryable, and we could simplify the queries here.
I think this issue is severe enough that would be worth doing, but it would be a lot more work - would need an upgrade path etc.
I had a look into the test failures.
The problem is in the way that config entity queries work.
The key value entries look like this:
| config.entity.key_store.webform_options | uuid:eba38bf8-26e1-49ef-87f8-5093b78cf9f8 | a:1:{i:0;s:33:"webform.webform_options.education";}
webform.webform.test_cards_javascript.yml has a very long string of JavaScript in it.
The key_store tries to put the full value of the javascript string into 'name' which is not possible given it's varchar 128.
If we truncated this to varchar 128 in core it should work, so I will open an issue to discuss that. This might need to be postponed but would prefer to see feedback for the core issue is like first, there might be other ways to handle this if that specific change to core isn't doable.
Committed/pushed to 11.x and cherry-picked to 11.2,x, thanks!
This doesn't apply against 10.6.x, do we need a backport?
OK I originally opened this but I didn't directly work on any of the code and it's had multiple people both reviewing and working on it.
Made one very small change suggested by @acbramley on the MR via the suggestions feature.
Didn't realise when I opened this that the current behaviour was actually causing bugs (as opposed to being confusing cruft) , but great that we can close those as duplicate.
Committed/pushed to 11.x, thanks!
I think it's a good step forwards to do the one method and not the other.
The baseline changes had commit conflicts, but I regenerated the baseline locally.
Committed/pushed to 11.x, thanks!
Committed/pushed to 11.x, thanks!
Committed/pushed to 11.x, thanks!
One comment on the MR - I would have expected the validation to mean there's no need to change any code outside AssetControllerBase.
Therefore I removed the test for an empty include parameter.
I think this coverage is probably worth having - if we change the validation logic, it would prevent a possible regression.
I think it's fine to directly remove the method here especially given it's not even called by core. If someone had subclassed the controller and overridden the title callback to use elsewhere (even this seems unlikely), their title callback will be used in that context anyway. There is absolutely no reason for the method to be called from any other context.
We also need Pierre to confirm that he'd like to take this on in the issue.
There's also some documents to read and confirm (again in this issue) before taking this on. I can't find the documentation for the documents... but there's links in this issue 📌 Add dcam as a subsystem maintainer of the Link module Active .
Apart from that though, it will be great to have subsystem maintainers for the asset subsystem. I was able to get a few long-standing issues in since around 10.1 but there's a lot of work still to do on it.
I've pushed a commit for #19 and that appears to fix things locally.
Determine if it's possible to break up the module into submodules so as to allow for both in-place and site-to-site migrations.
This sounds like a good idea to me. Keep all the in-place-agnostic migrations in the main module.
Move the in-place migrations to an lms_migrate_in_place module (or with a better name).
Add a new lms_migrate_site_to_site module with plugins that don't require any Opigno APIs.
@vensires yes unfortunately neither the H5P module nor Opigno have been well maintained for the past five years. I was added as an H5P co-maintainer for Drupal 10 compatibility and there has been very little activity from anyone else until very recently when more co-maintainers were added (who I don't know, but are making a promising start).
I've opened 📌 Use div embed regardless of H5P plugin Active and added both issues to 🌱 Path to Beta Release Active .
Adding two more issues.
Yes #108 make sense to me as well.
I was able to reproduce this by forcing the Opigno formatter to be ignored.
The remaining console error is this:
js_93TZSF55vBWPab2tg…Z-ZnJqSClJgBsNROm:4 Missing H5PIntegration settings.
(anonymous) @ js_93TZSF55vBWPab2tg…Z-ZnJqSClJgBsNROm:4
I think the remaining issue might be that drupal settings (at least on the site I'm testing on) is in the footer, and h5p-integration.js doesn't wait for anything to load. We could try moving that into Drupal behaviors maybe.
@illeace could you try the content you're trying to embed but also force the H5P formatter to use div rather than iframe embed?
I'm looking at https://h5p.org/node/99838 and wondering if we should try to remove the iframe support altogether, would allow us to entirely remove this code.
I'm not able to reproduce the h5p-shipped jquery 1.9 being loaded with a relative path. Uploading a screenshot of how it looks on a site running Drupal 10.5 + h5p 2.0.0-alpha5
The MR as it currently is looks mostly OK - left a comment asking for an inline code comment since I'm not sure where the non-string value comes from.
If this definitely fixes the issues @illeace has found, I would prefer to have reliable steps to reproduce the bug from a clean install, but apart from that and the code comment, would be OK with merging this and opening a new issue for the remaining console error you're experiencing.
Anyone else experiencing issues, please make sure you're on at least h5p 2.0.0-alpha5 and clearly specify whether your site includes Opigno, the LMS module, or neither, as well as the specific H5P activity type in use, the specific console error you're getting, and whether there are any 404s in the network tab.
Moving to needs work since there's an MR here.
Marking duplicate of the new plan issue which is covering similar ground https://www.drupal.org/project/drupal/issues/3540386 📌 Explore using more service closures to break deep dependency chains and load fewer services Active
I'm sure this hasn't been fixed. It's most likely that it will end up in contrib with history module before it is but we should move it there with that module.
Yeah there's still things to extract from this one. Let's make it a plan.
Thanks for the issue summary updates. I'm still findinf this issue a bit hard to follow since it feels like it's covering multiple situations.
It sounds like there is a remaining issue where core changes to these files will still fail scaffolding, and I think it would make sense to change that to a warning rather than a failure.
We might need a spin-off issue to then explore some of the other approaches to remove the warning.
From the other issue, there must be a way to either not use the is front page cache context for the breadcrumb (maybe we could use the route match from the main request only)?
Even if we can never generate a URL parameter like this someone could theoretically send one so it's worth accounting for it.
Could use some test coverage - there's existing coverage of invalid asset URLs so should be straightforward to add to that.
Thanks for the MR. One question though is why not make specifically the H5P bundled jQuery use an absolute path before it gets here since we know only that file is the problem here.
This jquery library is already being loaded by Drupal, so this script tag seems redundant (at least for me in this case).
Let's open a follow-up for this. It would be great if we can remove as much of this logic as possible and get closer to regular Drupal asset handling.
I think it's OK to just do the deprecation here as long as we have generic coverage of the deprecation mechanism.
It's still valid but it should probably be won't fix for core - the performance issues look unsolveable and it would have to be opt-in to avoid conflicts between implicit and explicit aliases.
smustgrave → credited catch → .
Changes always go into the development branch first.
@Gabor pointed out this is a duplicate of ✨ Add a basic but usable install profile for building a site from scratch Active and I think if we slightly repurpose that issue, that's true, so did that and closing this one.
Marking 🌱 Merge the standard and minimal profiles into something in between Active as duplicate after a quick discussion with Gabor. The only difference here would be merging the two existing profiles into one instead of adding a third one, but I think given the end goal profile is the same, it's probably fine to re-purpose this issue.
There's also an existing use case now that #30 would break.
Module A implements e.g. a field formatter plugin that should only be discovered when module B is installed, because it relies on features in in module B. Let's say it depends on a formatter base clase or trait that module B provides.
With HEAD, the field formatter that module A provides will only be discovered by the plugin system when module B is installed.
With the change here, it would be discovered whenever module B is a composer dependency. However, the base class or trait may rely on other code in module B other than the trait/base class - so actually trying to use it when module B isn't installed could result in fatal errors or it otherwise not working at all.
It also means module A would behave differently in three ways rather than two - when module B is installed, when it's required by composer but not installed, and when it's not required by composer at all.
There is no proposal here for how to provide backwards compatibility for that case or otherwise how it would be addressed.
Welcome to the PHP Language. Interfaces will/must be loaded by the parser to be implemented. This is no different than being dependent upon psr/log for the interfaces it provides.
The plugin system includes explicit support for discarding plugins that implement missing interfaces during discovery to support use cases like the one above - e.g. modules can ship plugins that will be discovered only when the module the plugin requires is installed. You're pretending as if this doesn't exist and no-one has thought about this before.
Loading the namespace into the autolaoder makes the module optional
It doesn't make it optional in the code base you're arguing for the exact opposite here.
What is the criteria for determining that this is fixed given the non-probabilistic output? e.g. what percentage of attempts before the MR generated extraneous code, and what percentage afterwards? The before and after videos show one attempt each, but that could be within the normal deviation of either code base.
Committed/pushed to 11.x, thanks!
I added credit for everyone here including tanushree gupta because they confirmed they were beaten to it on this issue in slack.
While this was tagged novice, extra entire test classes that run for no reason are confusing and waste CI time so it's good to resolve this one.
I think this is a duplicate of 🐛 Outbound path processors miss the route name Needs work , we should at least close that once this is in and add credit etc. - short for time today so did not check approaches between the two issues or anything.
If you choose to implement a dependency or even install a package on your Drupal site you need to have vetted the developer to be sure they will continue to maintain the software (or be prepared to dump them should it be required).
No it's not the same, because you're specifically talking about the case of a contrib developer making a deliberate choice to add a hard composer dependency on another contrib module, instead of making that dependency properly optional. If the dependency is genuinely optional, then version compatibility issues are easier to mitigate later. Saying 'it's the site owner's fault if they install a module' is not really applicable in the context of an issue against core specifically to make bad scenarios as described above more likely.
Committed/pushed to 11.x, thanks!
While you're very much missed, it has been great to see you doing lots of great things which aren't Drupal (or software) instead!
This is still relevant, could use the patch converting to an MR.
Scenario #2 from #30 should be solved by providing two plugin classes, the second one implementing the interface from module C. If you wanted this to be automatically used by existing config specifying the first plugin, then the class could be swapped out by a plugin info alter.
Having a composer dependency on a module that may or may not be installed can introduce problems if module C isn't compatible with newer Drupal core versions - the site won't be able to update, and it also won't be able to composer remove because that would lead to the fatal error again.
📌 Slowly, very slowly start OOPifying the render system Needs review just got reverted so we should move this discussion back over there. Don't want to mark this as duplicate just yet because it'll drop it out of everyone's 'my issues' but would be good to move the test coverage back into that issue and maybe the latest one or two proposed fixes in separate draft MRs or similar.
I've gone ahead and backported this to 10.6.x and 10.5.x, was borderline on 10.5.x but there's literally no API change or behaviour change here it's just improving the performance of a very specific database query.
From that link:
This is an empty package that depends on the current "best" version of
mysql-server (currently mysql-server-8.0), as determined by the MySQL
maintainers. Install this package if in doubt about which MySQL
version you need. That will install the version recommended by the
package maintainers.
So do you think that means that they update this to the 'best' version of MySQL each Ubuntu release, rather than updating it within an Ubuntu release? If so that's probably right but it's not how I read it the first time.
Looks good now without the package manager changes.
Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!
I think per 🐛 Element plugin object wrappers causing AJAX failures Active we should roll it back and re-commit it once we have a solution to that issue. I'm not around much this week though so probably can't actually do that.
We went back and forth on mentioning local in #8-#10 because by mentioning linux and windows local, it was excluding OSX (but also we don't need to tell people not to run production environments on OSX), but I think the wording in #20 is fine.
Thanks for the updates. ADRs are still pretty new to me so I'm not sure what the format/verbosity is supposed to be like overall, but this all looks correct now! The one thing I wondered about is if it needs more details about the trade-offs between flexible storage for inputs vs them not being queryable, but if that's more detail than intended seems fine to leave it implied.
I think the module will need to hook_system_info_alter() to mark itself as un-hidden when it's installed, so that people can uninstall it. That hopefully should work pretty well though.
Yes this makes sense, once we deprecate the default storage, we can keep it as-is through the deprecation process and remove it, without actually having to change it. Hopefully it all works out, this is one of the hardest things we've tried to deprecate relative to how difficult you would think it would be given it's just one piece of default config.
This looks good - I responded to and closed the open threads on the MR because in each case they didn't need any changes for me.
Committed/pushed to 11.x, thanks!
I think this is a duplicate of 🌱 Change type hints from string|MarkupInterface to string|Stringable Active
📌 Add an in-memory LRU cache Needs work is already in the 11.x branch, so once 📌 ContentEntityStorageBase::loadRevision() should use the static and the persistent entity cache like ContentEntityStorageBase::load() Needs work lands it will only be in versions that are using the LRU cache. I think this is probably duplicate of the LRU issue but moving to needs more info since it's not my project to close issues in.
Opened 📌 Revisit latest revision / ::getActive() param conversion behaviour Active too to revisit the logic that causes this to be called so often (and also causes various other problems).
Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!
I was surprised to see user_role_grant_permissions() but I see 📌 Deprecate user_role_grant_permissions() and user_role_revoke_permissions() Needs work is still going.
Committed/pushed to 11.x, thanks!
mb_check_encoding() shouldn't be called without arguments - that suggests NULL is being passed as a cache ID?
I can't find the core issue, but ✨ Support for core navigation experimental module Active has notes for a redesigned workspaces/navigation integration. So if this MR fixes other things but not workspaces, we should probably go ahead here and then fix workspaces in that issue (or open it if it doesn't exist).
It's unambiguously a significant improvement, but also this is a good reminder why we didn't add request timings to performance tests because the variation is just too much to get anything close to reproducible.
Also:
session -> session_manager -> database -> session_manager.metadata_bag
We should be able to avoid creating a database connection until we know we have a session cookie to check against. Dynamic page cache hits for anonymous users can theoretically be served without touching the database at all. Auth can if the an alternative session storage is used.