@ressa this one is a bit complex localize.drupal.org is still using the Drupal 7 module so it is vital that all work happens on that branch.
Preprocess ended up being included here so fixing the issue summary.
I guess the only issue with #59 is injectability - like injecting the class resolver is not the same as injecting a ConfigImporterFactory object. But I think it is a great idea. Factories usually make sets of things but the ConfigImporter is very much a single thing. And it gets around the whole discussion about the factory getter method name. @phenaproxima++
We start need to address the name. As I've pointed out we already have the pattern of get() from factories and we should use it here.
$config_importer = $this->configImporterFactory->createConfigImporter($storage_comparer); - repeating the words config importer twice here does not add anything. And choosing method names based on their searchability without taking into context the class name is not a rationale for the longer name.
My comment in #113 is explaining why I changed themeEnabledList to themeList and in no way should block an rtbc.
The changes made yesterday are very very low level and don't affect the nature of the API or even functionality - they were all about the performance implications of this change and minimising them and will have no impact on themes or modules.
I've been thinking that the main services should only get the kernel. And then they should use it trigger events via getting the event dispatcher from the kernel and then everything is always done with the correct services and container because the kernel is a synthetic service and is always the same object throughout the request / process - regardless of the number of rebuilds.
The test fails are very much related...
Symfony\Component\DependencyInjection\Exception\RuntimeException: Cannot autowire service "Drupal\Core\Config\ConfigImporterFactory": argument "$lock" of method "__construct()" references interface "Drupal\Core\Lock\LockBackendInterface" but no such service exists. You should maybe alias this interface to one of these existing services: "lock", "lock.persistent", "drupal.proxy_original_service.lock.persistent". in /builds/issue/drupal-3123491/vendor/symfony/dependency-injection/Compiler/DefinitionErrorExceptionPass.php:48
The reason we have to set $this->containerNeedsRebuild to FALSE is the following code in \Drupal\Core\DrupalKernel::initializeContainer
// If the module list hasn't already been set in updateModules and we are
// not forcing a rebuild, then try and load the container from the cache.
if (empty($this->moduleList) && !$this->containerNeedsRebuild) {
$container_definition = $this->getCachedContainerDefinition();
}
So setting this flag to FALSE stops us from trying to get a container definition. In tests this prevents us from trying to access the database when there is potentially none set up. We could remove the empty($this->moduleList) here but probably that's best for a follow-up with a CR etc.
The problem with themeEnabledList is that it makes the programmer wonder about moduleList. And we don't need to change that here. Because of how we review we often miss the context of the rest of the class (this was even more true in the days of patches and dreditor).
We need to fix resetSystem() to re-inject all the container services that we have as properties. And this is really awkward because we've made all the service properties read only - we're going to have to undo this.
The re-adding back of the _ function is good and the previous rtbc still applies.
Something changed in gitlab and the cache started to work. I think marking this one as fixed is fine.
I checked and 11.1.x and 10.x are not affected.
https://git.drupalcode.org/issue/drupal-3035288/-/jobs/7013308 is an example where we can't get cspell to pass.
This critical because this pipeline can fail even if there are no cspell errors due to this and once it starts failing in a pipeline it'll never pass. Hotel California!
Ie. there are at least 50,000 odd sites that would have been broken by this removal... https://www.drupal.org/project/usage/bootstrap →
@nicxvan yes - I linked to a theme actively used in Drupal 11 that uses the function https://git.drupalcode.org/search?search=_system_default_theme_features&...
There are others but that would involving linking to a search tool I don't like to publicise.
We can'[t remove the underscored function in a minor release. It is going to break sites unless contrib updates... see link. There are others...
This is great idea - but we should be adding some test coverage to ensure this is working.
We also need to test config entities - I just closed 🐛 Config entities don't show 'Usage' in operations Needs work as a duplicate (because this has more recent work).
This is a duplicate of ✨ Add "Usage" link to the entity operation links Needs review - let's merge efforts.
We should be adding test coverage and the MR can be updated to use constructor property promotion, autowiring etc...
We use PHPCS to control the coding standards on this project - the tests are current green - if you want to change how we deal with long array declarations feel free to re-open this issue but note I'm not partial to the rule and the standard has been relaxed recently anyway.
I think this could be further improved because the assert is really odd. But so be it.
This was fixed in 📌 Automated Drupal 11 compatibility fixes for entity_usage Needs review
Can we convert the patch into an MR so the pipeline is run.
I would also be fantastic to add a test - see \Drupal\Tests\entity_usage\FunctionalJavascript\ViewsTest::testViewsIntegration and tests/modules/entity_usage_test/config/optional/views.view.eu_basic_test_view.yml so we can ensure this functionality continues to work.
We need to merge in 8.x-2.x to get the pipeline fixes. What's the minimum 10.x version then for the module - we might need to update composer.json and entity_usage.info.yml
This fixes the problem by providing the missing update path.
Discussed with @catch and we agreed that we should remove the interface. The service is not an important one to be able to swap out or decorate and maintaining an interface implies that it is. When we started on D8 we were a bit interface-giddy… and it felt like there was a rule that every service must have an interface… I know I’ve been responsible for asking for interfaces in the past. But I feel this was a mistake. The class is not final so if you really really swap out you can extend and do that.
We keep the 'profile' stream wrapper. Profiles are planned to be deprecated but we're a very long way off being able to actually do it.
Yes but, they can be uninstalled and therefore not exist rendering the profile stream wrapper useless and site breaking. It needs to be removed so I have done that.
@godotislate good point - I've added a note to the CR.
Committed and pushed 8ac2dac793f to 11.x and b07aab80fb8 to 11.2.x. Thanks!
Created the follow-up 📌 Refactor EntityCrudHookTest to not use $GLOBALS Postponed
HEAD is now green again. I split this up is smallest possible units of change to make it easy to see the impact. The original MR on this issue was also fixing lots of 11.x deprecations which would make 10.x compatibility hard to maintain. I recommend we keep 10.x compatiblity till 12.x is at beta and then we release a new version based off 8.x-2.x that is 11.3 or 11.4 and 12.x compatible and handle all the deprecations there.
alexpott → changed the visibility of the branch 3551388-fix-phpcs to hidden.
alexpott → changed the visibility of the branch 3551388-fix-phpstan to hidden.
alexpott → changed the visibility of the branch 3551388-fix-cspell to hidden.
Let's break this down into two MRs - one for the linting jobs and then one for the tests.
Merged in HEAD on both branches... should be green and ready for review. Obvs we need ✨ Translation of config actions Active to land first
Used a post update so we got config sorting for free.
#9 is not correct - when you first install the module $config->get('local_task_enabled_entity_types') will return an empty array.
$ vendor/bin/drush en entity_usage -y
[success] Module entity_usage has been installed. (Permissions - Configure)
$ vendor/bin/drush ev "var_dump(\Drupal::configFactory()->getEditable('entity_usage.settings')->get('local_task_enabled_entity_types'));"
array(0) {
}
The reason this issue exists is because #2932559: Make it easier to access entity usage list of an entity → did not have an upgrade path. We could add one here. It'd be something like
/**
* Initialize the "local_task_enabled_entity_types" config value to an empty array.
*/
function entity_usage_update_8207(&$sandbox) {
$config = \Drupal::configFactory()->getEditable('entity_usage.settings');
$entity_types = $config->get('local_task_enabled_entity_types');
if ($entity_types === NULL) {
$config->set('local_task_enabled_entity_types', [])->save(TRUE);
}
}
#14 is an existing bug - see #1299966-27: String context not taken into account when retrieving a translation → - this issue does not fix it or break it. I've created #3553786: Extract context from t or trans filter in Twig template → to track it and hopefully someone will fix it.
It is IMHO valid to share memory caches between services
@berdir can you provide an example where this is true. From my perspective this would be a code smell and either point to one service over-reaching it's responsibilities or missing API.
You are literally testing the basically hardcoded string you have added in Request::create('http://' . basename($kernel->getSitePath()) . '/core/scripts/drupal');
For me this feels like:
$a = 2 + 2;
assert($a === 4);
The assert can be removed no? Like it's not really adding much afaics
Committed 7c964ee and pushed to 11.x. Thanks!
Given we have new interfaces and an API I've only put this in 11.x so it'll be part of 11.3.0
FWIW here's a link to Symfony's namespaced caches https://symfony.com/blog/new-in-symfony-7-3-namespaced-caches neat and a little inspiring here...
@berdir yes but... moving to using a cache for static properties is going to make clashes much much more likely, think property names and service names.- they are very likely to clash. And while nothing was enforced with drupal_statiic convention meant that the use of __FUNCTION__ lead to automatic namespacing.
@geek-merlin I think you are right we can do this as a follow-up. I think we could:
- enable Symfony's expression language for the container - I think there is an issue.
- add a withPrefix() that returns an wrapped memory cache that adds the prefix to cache keys (think get, set etc...)
- add a compiler pass to change cache.memory service injections to cache.memory->withPrefix(SERVICE_ID)
Happy for this to go back to RTBC - once we've adding docs about namespacing the keys something with a recommended convention - I'll open the follow-up to see if we can build this into the container automatically.
We just need a convention/pattern for cache IDs.
From @catch
drupal_static() was simple, functional, and extremely standard across core and contrib code.
From @jweowu
Have we got the bit about a convention/pattern for cache IDs in the CR and docs? I can't see it. Because I'm concerned about every thing to the same memory cache and getting some very unexpected results. It feels like we could do something smart here and somehow use the service ID as a prefix when we inject the cache.memory service into another service. Like some sort of very thin wrapper. This kinda of automatic namespacing could solve some very tricky bugs when this is widely adopted by contrib. Say what we like about drupal_static(__FUNCTION__, FALSE) - it handled namespacing really really well.
I've added CRs, a release note and update the issue summary. Once those have been reviewed I think we're good here.
cache.static shouldn't change to database cache with this though. That feels wrong.
I've added an API change to consider before we merge this.
Lolz this change happens for 11.3.x and not 11.2.x - so at least we're ready for it...
@phenaproxima this would cause way more problems than it is worth. The real issue in localise.drupal.org but we can work around that in potx 7.x branch and hopefully we can forget all about this when we drop support for that after localize.drupal.org is updated to modern Drupal.
Houston we have a problem. Symfony only added support for tagged scalar in 4.0 ... ie.
* added support for tagged scalars.
```yml
Yaml::parse('!foo bar', Yaml::PARSE_CUSTOM_TAGS);
// returns TaggedValue('foo', 'bar');
```
This means that the Yaml parser in 7.x-3.x doesn't work exactly the way we'd like it to and this is why the tests are failing on 7.x-3.x. Really annoying. Also we can't move to Symfony 4 (or better) because of localize.drupal.org.
I think we're going to have to almighty fudge this and detect if the string starts with !translate and do magic.
I have an MR for the POTX module to support extracting the strings from recipes - see ✨ Extract strings from Recipes Active
So looking in the PHPCS issue queue you find:
https://github.com/squizlabs/PHP_CodeSniffer/issues/3035 where the maintainer says:
This sniff only looks at one file at a time. While it would be able to detect this very exact case, it wouldn't be able to do so if you put your class definitions into different files. This is why the sniff only ever produces warnings; it's pointing out where you might want to check during a manual code review.
I'm going to close this as it's not something the sniff will ever be capable of doing. You may prefer to use a different static analysis tool for this sort of checking and leave PHPCS to look more at code formatting.
For me this means we should remove this rule from Coder and look to see if something better is implemented in PHPStan. Given this is rule is not able to remove false positives I do not think we should enable it for core.
FWIW I think we should be fixing the rule upstream. Some of the overrides here are not useless. If you change a default argument then it is not useless. If you change the visibility it is not useless.
I think we should be asking ourselves if the rule is fit for purpose in it's current state and if all it is going to do is create noise and extra mental work to ignore the skip lines.
I've credited @klausi - I think it is going to be hard to find everyone who has contributed to this over the years but I agree that @klausi certainly has.
We do have test coverage that is now failing on 8.x due to this - https://git.drupalcode.org/issue/potx-3553345/-/jobs/6961106.
7.x does not fail because we're using an older twig version there.
This is not going to fly. l.d.o is on some ancient stuff... gonna have to suffer and wait.
This looks good - it fixes the test and preserves compatibility with older versions of Twig. I've also backported the fix to the 7.x branch so that it is ready if we update twig there.
alexpott → changed the visibility of the branch 3528177-potx-doesnt-recognize to hidden.
To actually fix the tests we need to land 🐛 Potx doesn't recognize t filters in twig 3.21 Active too... but this unblocks stuff by reducing noise.
alexpott → changed the visibility of the branch 3553345-fix-tests-7.x-3.x to hidden.
The problem only exists on the 8.x branch - the 7.x branch is passing - see https://git.drupalcode.org/project/potx/-/merge_requests/36.
On the latest version errors have the message and error code - at least they do in my logs :)
Exception occurred while trying to send test email from Redacted <redacted@example.com> to redacted@example.com; bcc: redacted@example.com. Error code 403: Forbidden! {"message":"Domain mail.redacted.com is not allowed to send: Your account is on probation and domains are limited to 100 messages / hour. Account will be enabled in 652 seconds"}
In recipes it looks like we're going to mark translatable strings using a yaml tag - !translate/code> - see [#3313863]. I think we should use the same approach everywhere in the recipe.yml file but still support extraction of the defaults as add in #2.
In recipes it looks like we're going to mark translatable strings using a yaml tag - !translate/code> - see [#3313863]. I think we should use the same approach everywhere in the recipe.yml file but still support extraction of the defaults as add in #2.
alexpott → made their first commit to this issue’s fork.
This is ready for reviews - I’ll open the Potx issue to support this. And address @phenaproxima’s comments when I’m in front of a computer and not a phone.
Committed and pushed f4b0ecfb98d to 11.x and 2aa1f42b032 to 11.2.x. Thanks!
replicated in the installer.
This is not quite correct - they are replicated in the ConfigImporter - see all the code in \Drupal\Core\EventSubscriber\ConfigImportSubscriber