Account created on 23 January 2007, about 17 years ago
#

Merge Requests

More

Recent comments

πŸ‡¬πŸ‡§United Kingdom joachim

Yes, because this callback is failing somehow:

> 'callback' => '::getBundlesList',

That's the bit that needs debugging.

πŸ‡¬πŸ‡§United Kingdom joachim

No, that's a false error because the form gets submitted with broken values.

I need the error that causes an empty list of bundles for products.

πŸ‡¬πŸ‡§United Kingdom joachim

Can you try using the admin UI rather than Drush please?

I just managed to enable admin toolbar using that.

πŸ‡¬πŸ‡§United Kingdom joachim

Have you cleared caches? It turns out the extension system caches .info file data -- see the InfoParser class.

πŸ‡¬πŸ‡§United Kingdom joachim

That's a Composer error. This issue is not about Composer. It assumes you have already installed the package with Composer and you are now trying to install it in Drupal.

(To install it with Composer, use https://github.com/joachim-n/drupal-core-development-project)

πŸ‡¬πŸ‡§United Kingdom joachim

> The icon that I added is not the same as the browser(row-resize),

The row-resize icon has a horizontal line in it because it's about dragging a *boundary*, the line between rows.

Here we are dragging *objects*, the rows themselves. That's not the same operation and I don't think that's the right icon.

Something that's just the same as the 4-way arrows but only up and down would make more sense.

It would also be more visually distinctive, as currently with poor vision, both icons look like a + shape.

πŸ‡¬πŸ‡§United Kingdom joachim

> 2. Look at https://getcomposer.org/doc/04-schema.md#classmap instead of PSR autoload.

That's not going to work. I've tried it, and any Composer operation which happens before the DrupalLocation.php is written produces this error, multiple times:

> Could not scan for classes inside "DrupalLocation.php" which does not appear to be a file nor a folder

That happens when doing `composer drupal:scaffold` and probably will happen when first installing Drupal with Composer.

The problem is that 'classmap' autoloading looks in the given list of files to find classes to register. That's unlike 'psr-4' autoloading, which merely declares directories to Composer, and it will only look for files when a class is about to be loaded.

So with psr-4 it's fine to declare a file that doesn't exist yet, but with classmap it's not.

@alexpott - your concern was that the namespace could clash with other things. What if make it more specific?

Instead of:

> namespace Drupal\Locations;

we have:

> namespace Drupal\Composer\Locations;

The Drupal\Composer namespace is used by things in our /composer folder, nobody else should be using that namespace.

πŸ‡¬πŸ‡§United Kingdom joachim

I meant PHP breakpoints.

And just the lines where it goes wrong will do.

When you change the entity type, there's an AJAX callback which should be returning bundles.

πŸ‡¬πŸ‡§United Kingdom joachim

Aha, that's worked. Thanks!!

πŸ‡¬πŸ‡§United Kingdom joachim

So is the right way to do it:

1. merge 11.x into feature branch
2. Update lock file
3. switch to feature branch, commit changes
4. switch to 11.x and do a git reset --hard to put it back

?

πŸ‡¬πŸ‡§United Kingdom joachim

I'm trying to rebase this, and I'm not managing to update Composer from lock, and I don't remember how I did it previously!

> COMPOSER_ROOT_VERSION=11.0-dev composer update --lock

says:

Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Root composer.json requires drupal/core == 11.9999999.9999999.9999999-dev (exact version match), it is satisfiable by drupal/core[11.x-dev] from composer repo (https://repo.packagist.org) but drupal/core[11.0-dev] from path repo (core) has higher repository priority. The packages from the higher priority repository do not match your constraint and are therefore not installable. That repository is canonical so the lower priority repo's packages are not installable. See https://getcomposer.org/repoprio for details and assistance.

and

> COMPOSER_ROOT_VERSION=11.9999999.9999999.9999999-dev composer update --lock

works, but then the version set in composer.lock is all wrong.

πŸ‡¬πŸ‡§United Kingdom joachim

Thanks for the MR. It's a good start, but the lines need wrapping, and we should also say what the key is.

πŸ‡¬πŸ‡§United Kingdom joachim

> I am grateful you created that solution, but my suggestion is to do this directly in Drupal core.

My long-term plan is to get that Composer project template into core. There are a number of issues to fix first though. You could probably grab the code that allows Composer to allow installation of 10.x packages on 11.x and turn it into a core patch, but I think the work to do with Composer and the work to do with the extension system are best kept as two separate issues, as they're totally different systems.

In the meantime, I strongly recommend that anyone doing dev work on core use that template.

πŸ‡¬πŸ‡§United Kingdom joachim

Rebased and added package as per #25.

πŸ‡¬πŸ‡§United Kingdom joachim

> This should override both composer.json as well as *.info.yml limitations.

That's already fixed in https://github.com/joachim-n/drupal-core-development-project.

πŸ‡¬πŸ‡§United Kingdom joachim

Could you put a debug breakpoint in getBundlesList() and see what happens when Product is selected?

πŸ‡¬πŸ‡§United Kingdom joachim

Looks good so far, but method names such as getComponentNames need changing too.

πŸ‡¬πŸ‡§United Kingdom joachim

Could you do some debugging to see what's causing the error?

BTW, please don't add random tags to issues; tags have specific purposes. And it's better to copy-paste error text rather than take screenshots, as then the error text can be used to search the codebase.

πŸ‡¬πŸ‡§United Kingdom joachim

Tagging as novice, because test failures should be just a case of updating the expected exception messages.

πŸ‡¬πŸ‡§United Kingdom joachim

Figured it out with help from @berdir in slack.

Fixed, and new release coming soon.

πŸ‡¬πŸ‡§United Kingdom joachim

Fix looks good, but the comment has been lost, and unrelated fixes have crept in.

πŸ‡¬πŸ‡§United Kingdom joachim

How much more work does it need? I didn't think it needed to be extensively developed in contrib - could we just improve the code here?

πŸ‡¬πŸ‡§United Kingdom joachim

Yup, that would be the way I'd do it.

The entity that represents URLs would be used for local URLs as well, since routes on a Drupal site can be anything - views, controller classes, etc.

For a mechanism, I'd use Action Link module to create the links, and in the action link plugin create the URL entity if necessary and flag it.

πŸ‡¬πŸ‡§United Kingdom joachim

Hmm on second thoughts,

> Array of component lists indexed by type

does sort of describe it -- it's an array of several lists, and the array is keyed by the type of the things in the list.

The other issue is going to rewrite that text anyway so let's just fix the incorrect 'update' here.

πŸ‡¬πŸ‡§United Kingdom joachim

It would be really good if the code comment explained that a bit more, and more precisely, explained the circumstances under which the language code should be updated.

I think this logic could be made clearer:

          $langcode = $config->get('langcode');
          $is_available_langcode = in_array($langcode, $available_langcodes);
          if ((empty($langcode) || $langcode == 'en') && !$is_available_langcode) {

In the case that $langcode is empty, then it's obviously not in the array. So we only need to check if it's $available_langcodes if it's 'en', AFAICT?

So for example, this would be clearer:

          if (empty($langcode) || ($langcode == 'en') && !$is_available_langcode) {

And while I am generally REALLY in favour of splitting up complex conditionals, I'm not sure breaking out the check for $is_available_langcode is good for readability here, and it's not efficient, as it's checking even if $langcode is empty, or not 'en'.

I'm still not sure how we test the patch using the API.

I've installed book when 'fr' is the default language, with and without the fix, and looked at the {config} table.

Without fix:

	node.type.book	a:12:{s:4:"uuid";s:36:"10752ba3-9235-4540-a959-c9e9190376ae";s:8:"langcode";s:2:"fr";s:6:"status";b:1;s:12:"dependencies";a:1:{s:8:"enforced";a:1:{s:6:"module";a:1:{i:0;s:4:"book";}}}s:5:"_core";a:1:{s:19:"default_config_hash";s:43:"xvVZ9piiDxh4ziNyl-YqCT5vF8nI7xyupTdQWlN-Hxk";}s:4:"name";s:9:"Book page";s:4:"type";s:4:"book";s:11:"description";s:87:"<em>Books</em> have a built-in hierarchical navigation. Use for handbooks or tutorials.";s:4:"help";N;s:12:"new_revision";b:1;s:12:"preview_mode";i:1;s:17:"display_submitted";b:1;}
language.en	node.type.book	a:2:{s:4:"name";s:9:"Book page";s:11:"description";s:87:"<em>Books</em> have a built-in hierarchical navigation. Use for handbooks or tutorials.";}

With fix:

	node.type.book	a:12:{s:4:"uuid";s:36:"88e05137-2a0e-4a18-840e-60769f3dccd7";s:8:"langcode";s:2:"en";s:6:"status";b:1;s:12:"dependencies";a:1:{s:8:"enforced";a:1:{s:6:"module";a:1:{i:0;s:4:"book";}}}s:5:"_core";a:1:{s:19:"default_config_hash";s:43:"xvVZ9piiDxh4ziNyl-YqCT5vF8nI7xyupTdQWlN-Hxk";}s:4:"name";s:9:"Book page";s:4:"type";s:4:"book";s:11:"description";s:87:"<em>Books</em> have a built-in hierarchical navigation. Use for handbooks or tutorials.";s:4:"help";N;s:12:"new_revision";b:1;s:12:"preview_mode";i:1;s:17:"display_submitted";b:1;}

I can see there's a difference, obviously -- only one row rather than two!

But shouldn't I be seeing translated text in there?

I also don't understand what the API reports.

I've got this debug code:

$config_name = 'node.type.book';
      // enabled but not default language
      $override = $this->languageManager->getLanguageConfigOverride('en', $config_name);
      dump($override->isNew());

      // the default language
      $override = $this->languageManager->getLanguageConfigOverride('fr', $config_name);
      dump($override->isNew());

WITH the fix, I get FALSE, TRUE

WITHOUT the fix, I get TRUE, TRUE

I don't understand how BOTH are reporting they're new without the fix.

πŸ‡¬πŸ‡§United Kingdom joachim

I think we could expand the scope to include the other error that @Prem Suthar spotted, which is that the structure of the @param array is incorrectly documented:

> Array of component lists indexed by type

However, the current MR is removing the '(optional)' part.

I've filed πŸ› Fix incorrect use of word 'component' in locale module Active , which is a bigger task which covers both code and docs.

πŸ‡¬πŸ‡§United Kingdom joachim

Thanks for the MR!

Could you just fix the one problem for this issue though, rather than expand the scope?

I am going to be filing an issue to fix the confusing use of 'component' to mean extensions, but it's going to involve method renaming too.

πŸ‡¬πŸ‡§United Kingdom joachim

I'm digging some more in this.

I'm confused by the fix in the MR being in code that says this:

      // Update active configuration copies of all prior shipped configuration if
      // they are still English. It is not enough to change configuration shipped
      // with the components just installed, because installing a component such
      // as views may bring in default configuration from prior components.

'prior shipped configuration' means config that was installed from a module *before* the current install operation. So I don't see how that's going to help with the bug, which is about something that goes wrong with config from a module *as it's being installed*.

But then I'm not sure what updateDefaultConfigLangcodes() is trying to do anyway -- what does 'default' mean in this context? In getDefaultConfigLangcode(), 'default' means 'shipped with a module's code in config/install'. But here we're updating config, so it's not the shipped version is it?

πŸ‡¬πŸ‡§United Kingdom joachim

> when a form's submit button has the '#progress' set, neither the validateForm() nor the submitForm() methods get any values in $form_state->getValues().

The culprit is this line in the JS

> button.parentNode.innerHTML += throbber;

The moment the button is clicked, all the form elements get their values cleared.

I have no idea why that's happening.

πŸ‡¬πŸ‡§United Kingdom joachim

I've had a go at fixing the issue with other non-progress buttons causing the progress-enabled button to get the message.

That works... but with two progress-enabled buttons, the second one gets the message for either of them, so still not quite right :/

Reopening this as I think there's a use case.

πŸ‡¬πŸ‡§United Kingdom joachim

I think this would be a nice UX addition to various forms. In core, there's the permissions form and the content translation form which are both very slow to submit.

However, I've found a problem with the MR -- when a form's submit button has the '#progress' set, neither the validateForm() nor the submitForm() methods get any values in $form_state->getValues().

πŸ‡¬πŸ‡§United Kingdom joachim

> A module may choose to opt in any directory, including `src/` itself.

Can we make this automatic, rather than opt-in?

The opt-in code is just going to be repeated boilerplate.

I'd suggest we make it opinionated about where service classes are located, and allow modules to override if they want to. E.g.

- services in src/Services
- event subscribers in src/EventSubscriber

πŸ‡¬πŸ‡§United Kingdom joachim

This is because there's no config schema defined for the plugins.

I'm having a go at fixing it and it's got me stumped. I'll push what I have so far.

(BTW: Please don't add random tags to issues!)

πŸ‡¬πŸ‡§United Kingdom joachim

Removed the quoted code, replaced with a proper list that's aimed as users, not written from the code POV.

πŸ‡¬πŸ‡§United Kingdom joachim

> A specific plugin system can always define additional keys on top of this, like a replaced_by or something. For most other plugins that's likely not needed or more complex

Agreed - this is not going to be needed for most plugin types. I just wanted to make sure it'll be possible to add that on top of what's being done here. This issue is the last blocker to the entity links handler issue :)

πŸ‡¬πŸ‡§United Kingdom joachim

> In a way that needs to be understood by code and handled automatically, or only in a message to be read by humans?

By code.

Suppose node module defines a menu link 'foo'. It wants to replace that with 'bar'.

Contrib and custom code declare their own menu link items and say 'parent: foo' because they want to place their pages within Node module's admin UI.

The menu system needs to be able to go 'Oh, I'm going to use 'bar' as a parent for that item instead because 'foo' is deprecated'.

The use case is πŸ“Œ add an entity Links handler, to provide menu links/tasks/actions for entity types Needs work , where we need to deprecate hardcoded menu links/tasks/actions and replace then with automatically-created ones.

πŸ‡¬πŸ‡§United Kingdom joachim

Is this going to be extensible by specific plugin types?

For example, in the menu system, we need to be able to not just say plugin 'foo' is deprecated, but to use plugin 'bar' *instead*.

πŸ‡¬πŸ‡§United Kingdom joachim

> In latests versions for D10, the code seems embedded in the main module and force it.

No, it's not forced at all. It's the only license plugin that's OOTB. If you want something else, you need to create your own license plugin or install another module.

πŸ‡¬πŸ‡§United Kingdom joachim

Thanks!

πŸ‡¬πŸ‡§United Kingdom joachim

joachim β†’ made their first commit to this issue’s fork.

πŸ‡¬πŸ‡§United Kingdom joachim

IIRC each license type plugin can define what counts 'different' licenses.

πŸ‡¬πŸ‡§United Kingdom joachim

Why can't the dropbutton be made to be the size of its largest item?

πŸ‡¬πŸ‡§United Kingdom joachim

Thanks for the MR.

Good text -- I was struggling to think of how to reword the summary line! Needs a few tweaks.

πŸ‡¬πŸ‡§United Kingdom joachim

Ok I am now fairly sure that test coverage for this should be added to LocaleConfigSubscriberForeignTest. But I am very confused about how to make that work:

  public function testInstallModuleWithConfiguration() {
    // Install the Language module's configuration so we can use the
    // module_installer service.
    $this->installConfig(['language']);
    $this->container->get('module_installer')->install(['locale_test_translate']);
    $this->installConfig(['locale_test_translate']);


    // Do we need to do this?
    locale_system_set_config_langcodes();
    $langcodes = array_keys(\Drupal::languageManager()->getLanguages());
    $names = Locale::config()->getComponentNames();
    Locale::config()->updateConfigTranslations($names, $langcodes);

    // Not this -- it fails on both 11.x and the feature branch.
    $this->assertEquals('hu', \Drupal::service('locale.config_manager')->getDefaultConfigLangcode('locale_test_translate.settings'));

    // ???
    $this->assertEquals('hu', $this->configFactory->getEditable('locale_test_translate.settings')->get('langcode'));

My problem is that I don't understand the underlying architecture of how config is translated to understand what all the test helpers do and what the services like localeConfigManager / LocaleTranslation etc do. I suspect this is the case with most people and that the translation system has a very low bus factor!

πŸ‡¬πŸ‡§United Kingdom joachim

Tweaked mentions of D8

πŸ‡¬πŸ‡§United Kingdom joachim

I wonder whether the kernel tests in LocaleConfigSubscriberForeignTest are relevant here -- they seem to be testing the same sort of things.

πŸ‡¬πŸ‡§United Kingdom joachim

I can't work out how the translated node type label is supposed to show when you go to de/admin/structure/types/manage/page -- it doesn't work manually for me at all.

So I can't figure out how the actual test part of the test could be done using a kernel test, as I can't work out what's happening when it goes wrong. How does the translated node type label get loaded to be shown in the form?

In the meantime, I'm streamlining the Functional test with code from the kernel test I started writing.

I'm also removing the mentions of locale_test_translate -- that's not being enabled in this test, so it looks like frankencode comments to me :)

πŸ‡¬πŸ‡§United Kingdom joachim

This is a really good start! Needs some clarification on a few things though.

πŸ‡¬πŸ‡§United Kingdom joachim

With the current MR, going to admin/config/regional/language crashes with:

> Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException: Circular reference detected for service "router.route_provider", path: "options_request_listener -> router.route_provider -> cache_tags.invalidator -> maintenance_mode_subscriber -> url_generator". in Drupal\Component\DependencyInjection\Container->get() (line 147 of /var/www/html/repos/drupal/core/lib/Drupal/Component/DependencyInjection/Container.php).

πŸ‡¬πŸ‡§United Kingdom joachim

I wonder if the test be a kernel test rather than a browser test? I'm going to experiment with converting it :)

πŸ‡¬πŸ‡§United Kingdom joachim

> I think we should only include the ones we know for sure that exist.

Agreed.

If other keys might exist, we can say that, for instance (totally making this up) 'Further properties may be added by modules that implement hook_foo_alter()'.

If the return of all of these has the same format, I think it's ok to use a @see getAllViewModes() and only have the docs in one place.

Also see #2.

πŸ‡¬πŸ‡§United Kingdom joachim

ARGH WTF it's gone to the wrong branch.

πŸ‡¬πŸ‡§United Kingdom joachim

joachim β†’ made their first commit to this issue’s fork.

πŸ‡¬πŸ‡§United Kingdom joachim

Please file a new issue for a new problem.

πŸ‡¬πŸ‡§United Kingdom joachim

Clarified install/dependencies

πŸ‡¬πŸ‡§United Kingdom joachim

@shivam_tiwari I don't understand what you've done to the fork, as the branch is not showing any new commits.

And please see my comment -- the current patch is not useful.

πŸ‡¬πŸ‡§United Kingdom joachim

I wonder whether the configuration should happen at the field level rather than the bundle entity level. It would make this a lot easier in terms of the machinery needed to hook into core -- form alters, save hooks and so on.

It would also open up possibilities of being able to direct terms to different fields, e.g.

- terms found in body field 1 -> set in term field 1
- terms found in body field 2 -> set in term field 2

πŸ‡¬πŸ‡§United Kingdom joachim

That sounds like it could be useful.

> In a later step it would be even better to be able to edit them on this central page, but I think that will not be simple-

It would be fairly easy to make a link to the relevant 'Manage display' page, and append a redirect destination to when the user saves that form they come back to this central list.

πŸ‡¬πŸ‡§United Kingdom joachim

Pushed a bunch of fixes. Tests now pass.

Still to do:

1. My comment in #93. I'm not sure how best to resolve this and it would be useful to have input from the original author of that code. But it looks like the checking in Queue was added in #48 back in 2019, and the checking in BackendBase even before that, so I don't know if those devs are even still around. Given that, I'd be inclined to remove the checks in Queue and let backends use the ensureUniqueJobsSupport() helper.

2. getDuplicateJobId could probably be made protected. That would make SupportsUniqueJobsInterface an empty interface, but there's already precedent for that in the backend interfaces.

3. I REALLY want to rename the DB column we're adding from unique_id to something like job_hash. It's going to be a source of confusion and bugs with the current name. I appreciate that sites are already using this patch.... but that's a risk you take when using a patch.

4. The MR makes changes to existing hook_update_N() implementations, which is out of scope AND generally not a good idea:

-  $schema = Database::getConnection()->schema();
-  $schema->addIndex('advancedqueue', 'queue_state', ['state'], $spec['advancedqueue']);
+  $database_schema = \Drupal::database()->schema();
+  $database_schema->addIndex('advancedqueue', 'queue_state', ['state'], $spec['advancedqueue']);

These were first introduced in #51 -- and later patches have added the same changes to newer implementations that were added to the module in the meantime!

πŸ‡¬πŸ‡§United Kingdom joachim

I assumed since the classes we're moving are currently in lib/Drupal/Core, the new component would also be in lib/Drupal/Core.

There isn't a word for 'thing that's inside lib/Drupal/Core', AFAIK 'component' is used for both lib/Drupal/Core and lib/Drupal/Component.

Production build https://api.contrib.social 0.61.6-2-g546bc20