🐛 "Only file JavaScript/CSS assets can be optimized" errors in logs Active might be actually fixed through this approach.
These errors seem to come from the fact that Drupal is getting requests from old aggregates (possibly browser cache or other means?).
If I understand correctly, we would assert the old asset hash with the new one, which would fail and lead to "optimizeGroup" not being called at all, meaning no error would be thrown.
I created the two issues here 🐛 Setting "preprocess" false in "klaro_js_alter" could lead to unintentional problems with file aggregation Active and here 🐛 Setting "preprocess" false in "cookies_mySubmodule_alter" could lead to unintentional problems with file aggregation Active .
Alright, static patch for the time being. In theory, this could fix our problem, depending on whether "preprocess" is part of the hash.
I am still unsure how we end up in optimizeGroup, for a js library, that has preprocess set to false (even if altered through a third party module (e.g. cookies). But this might do the trick.
grevil → changed the visibility of the branch 3416508-improvejs-collection-optimizer-lazy to hidden.
Works great and as expected!
But we should investigate 🐛 $hook must be of type callable, string given, called in ModuleHandler Active internally.
Ok, got it now! It was the old problem, that adding a single tab won't display the Tab, as you need to add a separate tab leading to the form itself, for the translate tab to appear....
Please review!
Alright, I cleaned everything up and added a local task deriver to dynamically add a "Translate" tab to every entity extra field page instead of only providing it in the list builder actions tab.
But the route tab is not showing up yet. But I have one idea.
I'll see if I can replicate the error message with a clean Drupal installation.
Indeed, this might be related to 🐛 $hook must be of type callable, string given, called in ModuleHandler Active . But even when using patch #12 from that issue, the issue still persists, but with a slightly different error message:
Error: Call to undefined function \auto_entitylabel_entity_type_alter() in Drupal\Core\Extension\ModuleHandler->alter() (line 476 of core/lib/Drupal/Core/Extension/ModuleHandler.php).
I still think that core issue or one of the there mentioned middleware modules are the problem.
Updated the comment accordingly. Please see my comment in #43 regarding the "optimizeGroup" objection.
::optimizeGroup() should only ever run for files with preprocess: true, and there isn't an explanation here on how that ends up running for files with preprocess:false
Where is that defined in code? I can't find a single line of code in core, that suggests, that "optimizeGroup()" should only run for assets with "preprocess" set to false?
The flow is as follows:
The "system.routing.yml" defines "\Drupal\system\Routing\AssetRoutes::routes" as a route callback. In "routes()" "Drupal\system\Controller\JsAssetController::deliver" is used as one of the route controller methods. This simply references the parent method "AssetControllerBase::deliver()". And in there we run $data = $this->optimizer->optimizeGroup($group);, but "deliver()" doesn't have a single direct / indirect check on the "preprocess" property, so I am not really sure where this before mentioned check happens?
Yes, all fixed in 📌 Write tests for the module and its settings Active .
Ok, this issue escalated a bit, but I removed the weird config workarounds in the Block and adjusted the settings structure.
The update hook works as expected:
Ok, no idea the issue remains. Here are the steps to reproduce:
Error
The website encountered an unexpected error. Try again later.
TypeError: Drupal\Core\Entity\EntityTypeManager::Drupal\Core\Entity\{closure}(): Argument #1 ($hook) must be of type callable, string given, called in /var/www/html/web/core/lib/Drupal/Core/Extension/ModuleHandler.php on line 357 in Drupal\Core\Entity\EntityTypeManager->Drupal\Core\Entity\{closure}() (line 117 of core/lib/Drupal/Core/Entity/EntityTypeManager.php).
Drupal\Core\Entity\EntityTypeManager->Drupal\Core\Entity\{closure}() (Line: 357)
Drupal\Core\Extension\ModuleHandler->invokeAllWith() (Line: 117)
Drupal\Core\Entity\EntityTypeManager->findDefinitions() (Line: 216)
Drupal\Core\Plugin\DefaultPluginManager->getDefinitions() (Line: 25)
Drupal\Core\Plugin\DefaultPluginManager->getDefinition() (Line: 132)
Drupal\Core\Entity\EntityTypeManager->getDefinition() (Line: 257)
Drupal\Core\Entity\EntityTypeManager->getHandler() (Line: 192)
Drupal\Core\Entity\EntityTypeManager->getStorage() (Line: 97)
Drupal\Core\Datetime\DateFormatter->__construct() (Line: 259)
Drupal\Component\DependencyInjection\Container->createService() (Line: 177)
Drupal\Component\DependencyInjection\Container->get() (Line: 430)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters() (Line: 237)
Drupal\Component\DependencyInjection\Container->createService() (Line: 177)
Drupal\Component\DependencyInjection\Container->get() (Line: 430)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters() (Line: 273)
Drupal\Component\DependencyInjection\Container->createService() (Line: 445)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters() (Line: 237)
Drupal\Component\DependencyInjection\Container->createService() (Line: 177)
Drupal\Component\DependencyInjection\Container->get() (Line: 430)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters() (Line: 249)
Drupal\Component\DependencyInjection\Container->createService() (Line: 445)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters() (Line: 237)
Drupal\Component\DependencyInjection\Container->createService() (Line: 177)
Drupal\Component\DependencyInjection\Container->get() (Line: 430)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters() (Line: 237)
Drupal\Component\DependencyInjection\Container->createService() (Line: 177)
Drupal\Component\DependencyInjection\Container->get() (Line: 430)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters() (Line: 237)
Drupal\Component\DependencyInjection\Container->createService() (Line: 177)
Drupal\Component\DependencyInjection\Container->get() (Line: 430)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters() (Line: 237)
Drupal\Component\DependencyInjection\Container->createService() (Line: 177)
Drupal\Component\DependencyInjection\Container->get() (Line: 430)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters() (Line: 237)
Drupal\Component\DependencyInjection\Container->createService() (Line: 177)
Drupal\Component\DependencyInjection\Container->get() (Line: 430)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters() (Line: 237)
Drupal\Component\DependencyInjection\Container->createService() (Line: 177)
Drupal\Component\DependencyInjection\Container->get() (Line: 430)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters() (Line: 237)
Drupal\Component\DependencyInjection\Container->createService() (Line: 177)
Drupal\Component\DependencyInjection\Container->get() (Line: 430)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters() (Line: 237)
Drupal\Component\DependencyInjection\Container->createService() (Line: 177)
Drupal\Component\DependencyInjection\Container->get() (Line: 1467)
Drupal\Core\DrupalKernel->getHttpKernel() (Line: 715)
Drupal\Core\DrupalKernel->handle() (Line: 19)
Steps to reproduce
- Go to an entity's "Manage Extra Fields" Tab
- On the right side there is a drop down. Click it and select "Translate"
- Add a Translation
- Delete the Translation
Expected
The Translation gets deleted.
Actual
The Translation gets deleted, but a WSOD with the Error above kills the whole site.
Ok, the database error was caused by the WSOD, so it should definitly get fixed.
Ok, definitely not use this patch yet. After deleting and readding a translation for a bit, I now get a database error. This could be due to the config entity not being properly deleted, because an error is thrown on deletion.
Unfortunately, when deleting the translation, we get an WSOD on the WHOLE page, so this is definitly not ready yet. The WSOD goes away, after cache clearing, but still not good.
I occasionally get an AJAX Error,
That was due to non-existing file definitions in our customer's custom theme implementation.
Ok, we are now able to translate Each extra field through the action link:
I occasionally get an AJAX Error, when trying to translate the entities, but simply clicking again fixes the issue. I also don't quite understand the "EntityExtraFieldMapper" completely. I tried to strip it down as much as possible but there is still a LOT of code, which looks quite weird.
At some point "$this->entity" is not defined (when running drush cr) but otherwise it is necessary. "ProductAttributeMapper" in commerce doesn't give any more insight.
Something inside this module related to the routing is doing weird things, or this needs to be solved differently, but I wouldn't know how.
I'd say this is good enough as a temporary fix for the issue, but needs further work on the "EntityExtraFieldMapper".
Created a new release: https://www.drupal.org/project/choices/releases/2.2.0 →
Sorry, found a few more issues. After that, all should be good!
Sorry, this was added ages ago and we simply forgot to close this issue.
Alright, I added a test to show the issue. The tests only MR test pipeline fails with the expected "Exception: Only file JavaScript assets with preprocessing enabled can be optimized." exception and the MR with the fix + test is green!
Please review!
Sure thing.
1) Could be a good quick fix, but I'd go for 2) in the long run (if possible). 3) is probably not possible, since this "regular" js needs to modify the nextButton / prevButton via the Drupal.t() regex, BEFORE building and since we are not building dynamically, this will never happen.
I'd vote for 2). No idea, if we have any other options apart from the three.
Not sure, why this is a "replicate_ui" issue. The replicate ui only invokes the "replicateEntity" method from the replicate module. Meaning this should be a replicate issue.
Keeping this open for the RTBC'd D7 version of this issue.
Ok, I am now using the "starterkit_theme" instead of the "test_theme", since its label is wrapped in strong tags for some reason.
Also adjusted the summary logic slightly, to support the old deprecated "theme" config key.
Please review once again!
Note, that test failures are unrelated. The tests are simply not compatible with the newest PHPUnit version. We should fix that in a seperate issue.
@anybody, why have you removed the PluralTranslatableMarkup return type?
The "response_status" condition plugin has the exact same return type.
Ok, I created the MR and made the code a bit prettier. Please review!
I am a bit unsure about the output array:
$elements[$delta] = [
'#markup' => $video_url->toString(),
'#url' => $video_url,
];
looks a bit odd don't you think?
Ok nice! Adding the dependency in the info.yml is enough :)
All green! Merging.
Should work as expected, from "ProductAvailabilityDefaultFormatter":
// If the min / max delivery period is empty, we should use the global
// fallback:
if ($minDeliveryPeriod === NULL) {
$minDeliveryPeriod = $this->globalSettings->get('min_delivery_period_fallback');
}
if ($maxDeliveryPeriod === NULL) {
$maxDeliveryPeriod = $this->globalSettings->get('max_delivery_period_fallback');
}
I need to take a deeper look.
There was a problem with the issue fork. I pushed the changes directly to main.
Ok, I really don't get this whole switch case, or maybe I am missing something.
In line 110 we are already assigning the route parameters to the params:
// Massage the route parameters using the tour route parameters.
$params = $route_parameters;
Meaning, for the added test here, we already have "taxonomy_term: 1" now additionally we add in the switch case 'bundle' = 1 or now with the fix 'id' = 1. Both is unnecessary, since $tour->hasMatchingRoute will only check on "taxonomy_term: 1" the other added array values are completely ignored. That's why both tests succeed here, with and without the TourHelper adjustment. We could delete the WHOLE switch case and the test would still succeed, since the $params = $route_parameters before the switch case is enough, for this case to work.
Still changing the status to "Needs review" as at least it now fixes the incorrect "bundle= 1" addition.
Ok, that should be it. The approach wasn't correct. Sorry for the gitlab noise.
It is in general weird, that the default options differs from defineOptions to defaultExposeOptions. That is not the case in the parent class.
The description is referring to a boolean value which is kind of taxing for less technical users and should probably be avoided . And in combination with the label of the negate checkbox it makes things hard to comprehend. the cognitive load is rather high.
Adjusted the description accordingly. It now uses a similiar description to the "language" condition plugin.
add the menu link summary for consistency reasons.
This one is trickier than I thought. I have no idea, where this is defined in the condition plugin. I looked at other plugins but still no idea...
Interestingly enough, the "Vocabulary" condition has the same problem:
In general the preferable reading/processing order on those conditional plugins would be first what you want to do (aka the negate checkbox or the radio button pattern) and after that "what you want to apply it to"
This should be fixed in a general issue for all conditional plugins, since ALL of the use the same pattern:
Applied first
Negate second
you simply can not see it, since Blockform disables negation for all other condition plugins in line 253-265 of "BlockForm":
// Disable negation for specific conditions.
$disable_negation = [
'entity_bundle:node',
'language',
'response_status',
'user_role',
];
foreach ($disable_negation as $condition) {
if (isset($form[$condition])) {
$form[$condition]['negate']['#type'] = 'value';
$form[$condition]['negate']['#value'] = $form[$condition]['negate']['#default_value'];
}
}
IF it would be technical possible try to use a radio button component instead of the negate checkbox.
Same here, we should create a central follow-up issue for this as well. Since the "negate" behavior is baked in code through "ConditionPluginBase" "isNegated()" method:
/**
* {@inheritdoc}
*/
public function isNegated() {
return !empty($this->configuration['negate']);
}
This is not covered in the MR / Patches of [#14640540]. Therefore, I'd leave it as is for now.
I hope this is ok as is. Please review.
Postponed until ✨ Add a token for the site logo Needs work is even merged.
Created the follow-up issue here: 📌 Add alt text configuration for [site:logo] token Postponed .
I removed the alt text, replaced the deprecated "theme_get_setting" call and rebased the issue fork.
Thank you, @rkoller! Back to "Needs work" to implement the recommended usability adjustments.
I think I found the problem. Since the minification and build process is done through vite, there is no actual "Drupal.t()" in the processed tour.js anymore and the Drupal t regex won't match, which results in the string not being translated.
I tried turning off the minifaction in the vite config, but this results in further console errors AND the function becomes "Drupal2.t()" which could also still be a problem.
Unsure how to continue here, since we established, that we don't want to use the "normal" Drupal approach for the js library here: https://www.drupal.org/project/tour/issues/3548238#comment-16282931 🐛 Broken when using JS aggregation Fixed .
@smustgrave, what do you think? Any idea, besides refactoring the JS library to conform to Drupal standards?
I added the config_translation.yml. Everything looks ok, but I can not seem to get the Next / Previous Buttons translated, no matter what. I already translated "Next" and "Previous" in the User Interface translation, cleared all caches and ran cron several times to check, if there are new / multiple "Next" and "Previous" elements on the translation page, but no.
I also rebuilt the tour.js and added console.logs for the `tourStepConfig` to check if "Drupal.tour.nextButton" and "Drupal.tour.previousButton" works as expected, but everything looks fine. Seems like Drupal.t doesn't work in that context for some reason.
We should investigate this further.
Setting to RTBC for now, but still needs feedback by @smustgrave concerning #14.
Perfect, looks great! RTBC!
Added one comment, but unsure whether it is out of scope or not, but I don't like the new label. We should revert it and adjust it in a potential follow-up issue, OR fix it here and adjust the label as mentioned in the MR.