- On https://localize.drupal.org/translate/downloads -> 5xx server error.
On https://localize.drupal.org/translate/downloads?project=token I get the table with downloads.
This aligns with what I see in the code: The page needs a query parameter.
On https://localize.drupal.org/download?project=token I see the same.
Maybe a url alias?
The paths in the issue summary are a bit wrong.
In the code of the Drupal 7 module, in l10n_packager_menu(), I see 'translate/downloads' path.
In localize.drupal.org I get:
- On https://localize.drupal.org/download -> 5xx server error.
- On https://localize.drupal.org/downloads -> page not found
- On https://localize.drupal.org/translate/downloads -> 5xx server error.
- On https://localize.drupal.org/translate/download -> a list of translations.
- On https://localize.drupal.org/translate/abcdefghijklmnopqrstuvwxyz -> the same list of translations.
In 3.0.x in l10n_packager.routing.yml, I see path '/downloads', mapped to L10nPackagerDownloadForm which seems like a stub that does not do anything meaningful.
Both in D7 version and in 3.0.x, that downloads route is protected by 'access localization community' permission, which is defined in l10n_community.
In the Drupal 7 version, l10n_packager depends on l10n_community, probably for that exact reason.
I had a look at L10nPackagerDownloadForm which uses that permission.
It seems that form does not really do anything meaningful. It seems like stub code copied from a "how to create a Drupal form" tutorial.
I see that more work is done here,
📌
Finalize l10n_packager route /downloads.
Active
.
So maybe we should handle that first.
I pushed more changes which split the ModuleIntegrityTest.
Now we have a separate test for views, to detect broken handlers.
Maybe we should split things up, and only do the first step here which is about module install.
I find these other places with a very simple search:
core/tests/Drupal/Tests/BrowserTestBase.php
673: array_shift($backtrace);
core/tests/Drupal/Tests/BrowserHtmlDebugTrait.php
224: array_shift($backtrace);
core/tests/Drupal/TestTools/Extension/Dump/DebugDump.php
164: array_shift($backtrace);
170: array_shift($backtrace);
core/lib/Drupal/Core/EventSubscriber/FinalExceptionSubscriber.php
121: array_shift($backtrace);
core/lib/Drupal/Core/Utility/Error.php
112: array_shift($backtrace);
137: array_shift($backtrace);
core/includes/errors.inc
251: array_shift($backtrace);
I refactored the integrity test to use iterators.
It's a bit funky but imo it is the way to go.
I am now fixing a lot more stuff in this branch.
Because we can!
Now I get the same error on group/*/translate.
Beyond that:
I don't see anywhere in the codebase that we create a group type with that field!
It seems we rely on configuration that was created manually somewhere.
donquixote → changed the visibility of the branch 3553020-fix-phpstan-ci to hidden.
The MR fixes the indexing problem, but does not go further.
Setting to "Needs review", even though I don't think this is a complete solution.
Alternatively we could say that the initial scan should be done with drush, not the UI.
Another smart idea would be to process releases in reverse order (oldest first), stop after a given time limit, and go to the next batch iteration.
We can write the 'l10n_drupal_rest.last_sync_time' state value and in next iteration we can pick up there.
One question would be whether to download and parse the releases.tsv in every iteration, or keep it in the tmp dir.
It seems a good idea to keep the csv between batch iterations and delete it after the final iteration, not sure what could go wrong with this.
Problem 1:
The 'download_link' column is not indexed.
-> I am trying to have it indexed, by modifying L10nServerReleaseStorageSchema.
Problem 2:
Trying to have it indexed, I find the next problem:
The L10nServerReleaseStorageSchema class is not used, because it is misspelled in the L10nServerRelease entity definition.
-> I fix the entity definition.
With both of these changes, now the process is significantly faster.
Before: ~1 minute per 1000 releases.
After: ~2.5 seconds per 1000 releases.
(I will have to repeat this measurement, there may be some mistake)
The batch in the browser hits a time limit at ~130000 of the total 205061 releases.
But the process continues in the background.
Still this is not an ideal situation.
We could start with this simple fix but then keep the issue open to find a proper solution.
I identified the same changes when i looked deeper in the phpstan issue.
The '%extra' part in a url no longer works in that function since a05f31ecb70322cde2b from D7, even though other places with $extra still exist in that commit (but don't do anything useful).
https://git.drupalcode.org/project/l10n_server/-/commit/a05f31ecb70322cd...
Hi,
the main goal was to make tests pass equally in D11 and D10.
Regarding the "II", I find the following when I run with `--display-all-issues`:
- "Metadata" (annotations) in doc comments are deprecated, we should use attributes.
(We should not change this as long as we support Drupal 10) - 2 incomplete tests.
There were 2 incomplete tests:
1) Drupal\Tests\l10n_migrate\Kernel\MigrateL10nServerGroupTest::testRowsComplete
/var/www/html/l10n_migrate/tests/src/Kernel/MigrateL10nServerGroupTest.php:114
2) Drupal\Tests\l10n_migrate\Kernel\MigrateL10nServerGroupTest::testDatasetsComplete
/var/www/html/l10n_migrate/tests/src/Kernel/MigrateL10nServerGroupTest.php:121
This is literally in the test itself:
/**
* Tests migration for completeness.
*/
public function testRowsComplete(): void {
$this->markTestIncomplete();
}
/**
* Tests migration for completeness.
*/
public function testDatasetsComplete(): void {
$this->markTestIncomplete();
}
I think we should fix this in a separate issue.
First one introduced in
commit a93acb125c9072eeec4bb2a1bcce2b0eefc8a5cc
Author: Felip Manyer i Ballester <git @ res-telae.cat>
Date: Fri Nov 11 17:18:19 2022 +0100
Issue #3282696: Port of l10n_community: translation form (submit)Second one introduced in
commit 8a11aff3155bce7c21ed7f7a9aad751478ab1c92
Author: sanduhrs <stefan @ auditor.email>
Date: Thu Jun 23 08:45:36 2022 +0200
initial draft of l10n packager
Third one introduced in
commit a953d29ecdb1be75c6db2355dd1ee0ec3e0cc25a
Author: Felip Manyer i Ballester <git @ res-telae.cat>
Date: Fri Aug 23 10:42:52 2024 +0200
Replaced "Contributon" by "Contribution"I created follow-up MR here.
Maybe a new issue would be the "correct" thing but I don't want to pollute the issue list.
2. If we include that early will we have other issues?
People do weird stuff in .module I think we have to be careful of.
I suspect that this include already happened as a side effect of discovery in the past, as in my previous comment.
Let's confirm it doesn't solve the artificial problem, but should we explore solving an artificial problem? Are there other issues you forsee?
The artificial problem is more reliable to reproduce.
In the other variation, we are relying on accidental correctness, which is not good. The cache is saving us, but for the wrong reasons.
Actually, this problem might be a side effect of using static reflection parser in HookCollectorPass to collect procedural implementations.
In earlier versions, I think we did include the *.module files to do the discovery.
Actually, I found that container_rebuild_required did not help during site-install.
The cache is not enabled during site-install.
Even if the module has its own batch, it still triggers the error.
1. If you convert those hooks does the problem not occur? Is this a procedural only issue?
I would expect that the problem only occurs with procedural implementations which are not part of class autoloading.
2. If we include that early will we have other issues?
Normally a *.module file should contain only functions and constants, and possibly more include statements, hopefully with __DIR__.
In
#3144354: ModuleInstaller loads .module and .install before allowing classes to autoloaded →
we learned that we need the autoloading to be initialized for the module, so that constants can be delegated to a class.
All of this should be fine with the change proposed here.
There are some hooks we just don't support in middleware, this feels slightly analogous with that timing.
The problem is that hooks can be invoked indirectly. The component that invokes the hook might have no idea from where it was called.
As I mentioned, a lot can depend on warm vs cold caches of various kinds.
Another question, does setting container rebuild required change things? I don't think it will but should be an easy test.
It does help to avoid the problem in my original scenario, which only occurs during site-install, I suspect that caches are not really working during that.
It should not help with the artificial problem in "steps to reproduce", because that example is designed to occur even when only that one module is being installed.
We could also ask why we need to determine a language (LanguageRequestSubscriber) at this specific time during module install or site install, with all of the language negotiation plugins.
But in theory, any module could subscribe to this event and invoke a hook.
I can only reproduce the original scenario with entity types in a "drush site-install", with an install profile with a bunch of dependencies.
This is because something is always warming up the cache otherwise (with entity types based on the old module list).
However, the artificial second example in "steps to reproduce" is not impacted by cache, so it should be reliably reproducible.
I am putting this to "Needs review".
But before we merge: Maybe some of this can be done in separate issues - TBD.
One change is here:
🐛
PickGoForm::pickGo(): Missing return if $l10n_groups is empty
Active
We should merge that first.
In HookCollectorPass we introduced an empty line.
I think this is not intended.
protected function getOrderOperations(): array {
$implementationsByHook = $this->getFilteredImplementations();
$operations_by_hook = [];
foreach ($this->orderOperations as $hook => $order_operations_by_weight) {
ksort($order_operations_by_weight);
$operations_by_hook[$hook] = array_merge(...$order_operations_by_weight);
$order_operations = array_merge(...$order_operations_by_weight);
foreach ($order_operations as $key => $operation) {
if (!isset($implementationsByHook[$hook][$operation->identify()])) {
unset($order_operations[$key]);
}
}
$operations_by_hook[$hook] = array_values($order_operations);
}
return $operations_by_hook;
}
Sorry that I did not pay attention to this in the recent months.
/**
* Returns the identifier for filtering.
*
* The hook implementation identifier, as "$class::$method", to be changed by.
*
* @return string
* The identifier for the OrderOperation.
*/
This looks a bit strange.
It seems as if the "The hook implementation identifier, as "$class::$method", to be changed by." describes the return value.
But even then it is strange, what does "to be changed by" mean?
In the constructor doc, the $identifier parameter is described as "Identifier of the implementation to move to a new position.". It would make sense to replicate this here.
The "for filtering" is also confusing.
Nothing in the class file mentions anything about filtering.
We can either not talk about filtering at all, or we have to explain what it refers to.
The part in the return, "The identifier for the OrderOperation." is also misleading, because the identifier is for the implementation, _not_ for the order operation: There could be multiple order operations that target the same implementation and would have the same return value in ->identify().
/**
* Gets an identifier for the target implementation.
*
* This is used in HookCollectorPass, to remove order operations where the target
* implementation does not exist at all, or is not registered for the hook that the order
* operation targets.
*
* @return string
* Identifier of the implementation to move to a new position. The format
* is the class followed by "::" then the method name. For example,
* "Drupal\my_module\Hook\MyModuleHooks::methodName".
*/
One could also consider to rename the method to something like ->getTargetId() or ->getTargetImplementationIdentifier(), but I don't mind if we keep ->identify().
We could add something about nested calls and expressions where some levels are multi-line and others are not.
This would be more generic than the proposed section about options-style arrays.
foo($x, bar(
$y,
), baz(), [
'key' => 'value',
]);
I am not sure we allow this..
#5++
I don’t think the potential parsing errors are a good argument, but the diffs definitely are.
We’d just have to include the condition that this only applies if the code in question depends on PHP 7.3+ – which Drupal 7 code does not always do.
I think we can forget about Drupal 7 now :)
Although I don't mind having a note "trailing commas in function calls are available since php 7.3".
I was looking for an issue about this trailing comma, I fully support it.
As for the options proposed in the issue summary: I prefer option A, but maybe we can reduce the scope to only function arguments, not chained calls. The nested array argument could also be in a follow-up.
For the record:
When using "ddev symlink-project" from ddev-drupal-contrib I now get this:
PHP Fatal error: Uncaught RuntimeException: Positional parameters are no longer used. Ignoring ******* in /var/www/html/symlink_project.php:34
Stack trace:
#0 {main}
thrown in /var/www/html/symlink_project.php on line 34
I was going to post in ddev-drupal-contrib queue but somebody else was faster:
https://github.com/ddev/ddev-drupal-contrib/issues/154
Why exactly did we remove the project name as positional parameter, if this change was about recipes?
I think there was some reason why I did not do this originally.
So one thing we cannot do here is to add the assert instanceof as part of the ::service() method.
I think there was some reason why I did not do this originally.
But it does seem to work correctly.
https://phpstan.org/r/4b39b30f-7165-44db-b464-1b9b1312c10f
So I support it!
I see this in cspell output:
[WARNING] The project CSpell configuration does not contain all of the default flagged words. The missing words are:
blacklist, blacklists, blacklisted, e-mail, grey, queuing, whitelist, whitelists, whitelisted
It is recommended to add these words to your custom .cspell.json file.
See change record https://www.drupal.org/node/3524446 for more details.
Also if we fix this we should make it required, no?
Btw I don't really know if we need that "enforced" part.
dependencies:
enforced:
module:
- comment
vs
dependencies:
module:
- comment
See https://www.drupal.org/node/2404447 →
From how I understand it:
If we would let Drupal save the migration config entity, then dependencies are re-calculated. Any non-enforced dependency can be lost, and new non-enforced dependencies could be added, based on the plugins being used.
Having the dependency "enforced" means that we are fully in control, and Drupal is not allowed to remove it when it re-calculates dependencies.
However, for migration config entities, we typically don't allow Drupal to do anything, we only edit them by hand. At least this is how I remember working with migrate_plus migration config entities.
So in our case there is no practical scenario when the "enforced" would make a difference. But it is still safer and more correct, if we manually add a dependency, that we add it as "enforced".
The question was raised by Guiu what happens to the migration when we uninstall and reinstall comment module.
It is quite simple, because the migration is in config/optional:
(that is if the dependency is present as in the MR)
- When we uninstall comment module (if no migration has started yet), the migration config entity is gone.
- When we install comment module, while l10n_migrate is enabled, the migration config entity comes back.
I have not checked what happens if there is already a migration based on the config entity.
In my own experiment, the migration would not work anyway because I did not set up the source database.
However, there are some other observations:
- The l10n_migrate module currently depends on migrate_tools via l10n_migrate.info.yml. That module is not present via composer.json require-dev. This should be investigated further.
- To uninstall comment module, one must first uninstall fields that reference comments.
This said, I don'T see that any of the above would be a blocker to adding this dependency in the migration.
I see nothing in this issue that would be specific to Drupal 7.
It can still be a good idea to explain on the module page what this module does that cannot be easily achieved in other ways.
donquixote → made their first commit to this issue’s fork.
donquixote → changed the visibility of the branch stylelint-not-optional to hidden.
Review the current PHPCS configuration.
[..]
Update the PHPCS configuration file if necessary to enforce the standards consistently.
[..]
Code adheres to the agreed-upon coding standards.
It is using the default phpcs rules used for all projects.
We don't have custom phpcs rules for this module.
Fix or refactor code to comply with the defined coding standards.
[..]
Ensure that the CI pipeline runs PHPCS and passes without errors.
[..]
PHPCS reports no violations in the codebase.
[..]
The CI pipeline passes successfully with PHPCS checks.
done :)
Ensure that the CI pipeline runs PHPCS
We can see in upstream pipeline that phpcs runs and fails..
The MR is super unimpressive, and that is how it should be :)
@tstoeckler you can comment (or better yet, review) on the other issues, then we can give credit.
I think we it won't be possible otherwise.
Btw the unused import we may have to restore later, when we inject the service.
But that's ok.
Pipeline passes for phpcs.
The phpcs check is set to required already:
phpcs:
allow_failure: false
donquixote → made their first commit to this issue’s fork.
donquixote → made their first commit to this issue’s fork.
You can still use the attributes, you just need to phpstan ignore next line to ignore the missing attribute.
If we do this, phpstan will complain about "No error to ignore is reported on line **" when you run it with Drupal 11.
So maybe use a different baseline file per core version? Is this possible for a contrib module, in a way that Drupal CI will understand it?