> added 'locked' and 'default' config keys which I mapped to #disabled and #default_value
'locked' makes sense to have in schema too, yes.
But for 'default', surely the default value is already in config/install, so adding it to the schema violates DRY.
> Some will be be about how to correctly enter the value in the widget offered by the UI - which I don't think should live in the schema. In those cases, I would expect the form to override / add to any description coming from the schema.
Yup, agreed.
element:
'#description': Put here the main value.
'#weight': 42
'#attributes':
class:
- my-field
Hmm I'm not keen on this -- it's overloading the schema with things that go beyond its designed intention, which is to declare the structure and format of config. I can see a case for adding a 'description' key to go alongside 'label', but things like weight and classes are too much I think.
It's a JS test, which tend to be glitchy, and it's an area that's not at all related to Views, so it's not anything to do with this MR.
Pushed my WIP branch to a fork -- sandbox-3477363-automatic-config-forms
I keep forgetting to push the work I did on this so far.
I converted a few of the core settings forms, and found that in most cases, the form's structure is rather different from the structure of the config, which requires some tweaking of the automatic form code. The Views settings form is particularly bad for this!
I didn't say I'm against the idea of having a specific permission, just that the name needs work.
The caching per-url is definitely going to hurt performance.
But the reason appears to be this that's getting added into the toolbar:
'#attached' => [
'library' => ['responsive_preview/drupal.responsive-preview'],
'drupalSettings' => [
'responsive_preview' => [
'url' => ltrim($url, '/'),
],
],
],
with the $url coming from
$url = $this->getPreviewUrl();
where the value is the current URL, e.g.
Object { url: "node/6506" }
So changing the cache context to 'url.site' means that the first hit to build the toolbar will cache that specific URL, and all subsequent page loads will show the cached data with that URL. That will mean you'll get the wrong page opened in the preview!
The right way to fix this is to either:
- fix the JS so it doesn't need this URL in the settings -- I don't understand why we need to store a value for the current URL, when we can just get it from **the current URL**!
- use a lazy builder for the responsive_preview toolbar item
Here's a rough start.
Needs work, as other processors call importUrl() too, and they're not always reference fields, so the new parameter being $field_internal_name doesn't always make sense, and nor does the "omitted from reference field @field" part of the log message.
Need to figure out how to make this generic.
@anybody is right, 'settings' doesn't make sense here as there are no settings.
Not sure 'reports' is right either though, as field tools pages let you make changes to fields.
> I don't think this is an Entity Share problem :)
Agreed :)
I'm just curious what other people have come up with for managing this scenario.
I think we can say this is outdated, as annotations are being replaced by attributes, which *do* support wrapping and so on.
I'm inclined to say this should be a wontfix, as config entities can be deployed with the config system.
joachim → created an issue.
This is a bug because things not being configured yet shouldn't cause a PHP warning.
It's still really hard to get people to review MRs.
I think what I originally wrote for this issue is unclear.
What I was trying to say was that one of the following happen:
1. There is config data already: a LanguageConfigOverride object is created, which holds this data
2. There is no config data: a LanguageConfigOverride is created anyway, which is empty.
So it's not so much that 'if there's no data, an object is created', rather 'an object is always created; if there's no data it will be empty'
public function getOverride($langcode, $name) {
$storage = $this->getStorage($langcode);
$data = $storage->read($name);
$override = new LanguageConfigOverride(
$name,
$storage,
$this->typedConfigManager,
$this->eventDispatcher
);
if (!empty($data)) {
$override->initWithData($data);
}
return $override;
}
> field (looking in core, this is "EntityField" class at web/core/modules/views/src/Plugin/views/field/EntityField.php)
That one.
computed_string_field and computed_bundle_field are not a views field plugin IDs, they are just the name of entity fields in a test.
Drupal Code Builder could call it to check which hooks can be generated as class methods and which must be procedural.
Also, API module could use it to automatically document that about hooks.
Submodules aren't installed as packages by Composer -- Composer merely sees them as files inside the main module package. So there is no point having a composer.json file there.
I'm not sure why Drush is saying that, but it sounds wrong to me.
In any case, IIRC the extra.drush.services thing is obsolete in newer versions of Drush anyway.
Does something for this need to be added to the core BC policy too?
It's been pointed out to me on slack that LAG/LEAD are only available with MySQL 8. I've added docs in the README and also UI text for that option to say that.
Looks good, thanks!
> and you're left with flagging entities each time a user flags,
I think that creating flagging entities only to delete them is rather inelegant! My opinion on Flag is that you should only use it if you actually want what it does -- storing the flaggings.
It wouldn't be hard to create a custom Action Link link style plugin that extends the confirmation form to show additional form elements.
Is this bit:
> id = "computed_changed_shortterm_fields"
what you've declared to views data? 'id' is the ID of the Views field plugin to handle the field. And you've not defined that plugin. IIRC you can just use the plugin for the specific field type -- see the core issue for declaring bundle fields to views.
> I want to change the registration status, when the author of the node (not registrant) click on a link with a confirmation page.
There is already an Action Link plugin for toggling the value of a boolean field. So you could just create an action link that uses that plugin and configure it to work on your registration status field.
The latest alpha release added a link style that uses a confirmation form.
I'm not sure about the combination of node, user, and registrant.
Yup, a config entity which is only visible as an admin form doesn't need to provide a canonical link template or a canonical route. Core's config entity types don't, for example.
> 'node' instead of 'node_field_data',
node is the base table, node_field_data is the field data table. It may be that one of those is the old way, I'm not sure. Check how Views declares config fields to views data.
> no 'entity field' line.
IIRC Views field handler plugins expect this property to be there, to tell it the name of the field.
> The handler for this item is broken or missing. The following details are available:
Can you debug to see what handler it's trying to use?
Committed, with a small fix to the docs fix -- should be 'null' in lowercase in docs.
Thanks!
\Drupal\computed_field\Field\FieldStorageDefinition is for bundle fields defined in code, so it's correct that it doesn't have third-party settings, as those are for config entities. The other module is making an incorrect assumption about bundle fields -- see my comment over there.
We don't need to sort this query at all.
This is still the case on D11.
The query is:
SELECT t.*
FROM
node__body t
WHERE (entity_id IN ('1', 2)) AND (deleted = '0') AND (langcode IN ('en', 'und', 'zxx'))
ORDER BY delta ASC
and EXPLAINing it still shows a filesort.
I was right in #19 that we could add the entity_id to the sort to get rid of the filesort. But better still, as in the OP, we don't need the sort at all: the values from this query are added to a $values array which already has the entity IDs (or revision IDs) and is already ordered.
So we don't even need to replace the SQL ordering with PHP ordering -- no ordering is needed at all.
This is an incorrect assumption:
$is_base_field = $storage->isBaseField();
// Check if the field is encrypted.
if (
($is_base_field && $storage->getSetting('field_encrypt.encrypt')) ||
(!$is_base_field && $storage->getThirdPartySetting('field_encrypt', 'encrypt', FALSE))
A field that is not a base field is not necessarily a config field. Bundle fields can be defined in code too.
Rather than doing
> method_exists($storage, 'getThirdPartySetting
in the MR, check for whether the field is a ConfigEntityInterface.
What does this module do for base fields, which don't support third party settings either?
> i will note that Module Builder does not currently composer install for me, Drupal 11 and PHP 8.4 and maybe something else causing incompatible dependencies
Could you file an issue on Module Builder for that please?
Views displays are the majority use case of search API displays, surely. Even if there are properties which aren't used in all cases, they should still be documented.
Without this documentation, it's harder for developers to figure out how to correctly declare a custom Views display type to SearchAPI.
Perfect, thanks!
Oh wait, the hook attribute takes a short hook name:
> #[Hook('field_type_category_info_alter')]
which means the method probably does too.
We should document it's the short hook name, without the 'hook_' prefix -- see the docs on the Hook attribute class for an example.
Looks great, thanks!
EntityForm::buildEntity() is still called from afterBuild()
> Also may want to check usage as I don't see anything in core besides a test using this hook. Maybe it's not needed?
It's standard for all plugin managers to have an alter hook for the discovered plugin info.
joachim → created an issue.
> and it turns out the only issue is that the paths weren't prefixed with a preceding slash, so the path was appended to the path of the previous request resulting in 404s
Can you explain a bit more about where this path appending thing happens? Is it something we can maybe fix?
Though OTOH, since the Drupal convention is now that paths should have the initial slash, it's probably a good thing to add that consistently.
I've had a look at your patch on the other issue for converting BlockContentPageViewTest. I'm honestly confused by what this failing test is doing anyway:
public function testPageEdit(): void {
$block = $this->createBlockContent();
// Attempt to view the block.
$this->drupalGet('block-content/' . $block->id());
// Ensure user was able to view the block.
$this->assertSession()->statusCodeEquals(200);
$this->drupalGet('/block-content-test');
$this->assertSession()->pageTextContains('This block is broken or missing. You may be missing content or you might need to install the original module.');
}
Why would the block report as being broken? What's causing it to be broken? This test needs to explain what it is doing!
@mstrelan could you try adding this to the top of AssertContentTrait::getTextContent()?
// If this test uses \Drupal\Tests\HttpKernelUiHelperTrait to make requests,
// then get the content from the Mink browser.
if (isset($this->mink)) {
return $this->getSession()->getPage()->getText();
}
Thanks for having a look!
AssertContentTrait wasn't change by this MR.
I'm not sure whether to:
a. call AssertContentTrait::setRawContent in drupalGet()
b. change getTextContent() to use Mink, since that's now available? And then we can later on deprecate getTextContent().
> but I couldn't get the foobar_gorilla block to load in ::testPageEdit
Could you push your work on that test somewhere so I can have a look at it?
> The first blocker I couldn't easily work around was that the default cache policy for DPC prevents caching if PHP_SAPI is cli
The docs on HttpKernelUiHelperTrait::drupalGet() say that page caching modules won't work. Should that list of warnings be moved from the method docs to the trait docs, so they're more visible?
Using modules for feature flags feels wrong to me. It's piggy-backing onto an already complex system, which lots of things already talk to.
We'd be adding empty modules, which then all the various discovery services have to waste time checking -- plugins, routes, services, permissions, etc etc.
> - you have to add the configuration schema,
Wouldn't we have one central config for the feature flags, and so only one schema? And they config values are validated by being valid feature flag plugin IDs. (Or we could just cheat and declare the array to be of type 'unknown' :p)
> and an update path to set the default 'off' state of the flag
Wouldn't we have to do that with modules too, to enable them?
Or what about having a system where when updates are run, any discovered feature flag is set to 'on' unless it's explicitly 'off' in config?
> - you have to add a form somewhere so site owners can change the flag. This would either require a central 'feature flags' page or it would be dotted around the UI.
That's just one form.
> - when the feature is eventually enabled by default, it requires another schema change and another update path to remove the configuration.
We'd need that with module too, no?
The other downside of using modules is that it's messy for the admin user. With a separate feature flag config and UI, there is a clear and simple way for a site admin or site developer to answer the question 'Have I got any enabled feature flags?' With modules, how do you find that?
Merged, thanks!
Yes, it's still relevant.
The documentation around the NULL/[] difference for target bundles was updated in another issue about a year ago. IIRC it got fixed, so there should be an explanation in code now.
Tagging as NISU, as only option 2 is currently outlined in the IS.
I think that if 📌 Move source_module and destination_module from Migrate to Migrate Drupal Needs work makes the original proposal possible, we should wait for that, as that has the best DX IMO.
In core's autocomplete.js:
const options = $.extend(
{ success: sourceCallbackHandler, data: { q: term } },
autocomplete.ajax,
);
$.ajax(this.element.attr('data-autocomplete-path'), options);
We'd need to override autocomplete.ajax to include something like
data: { q: term, *something else here } },
Thanks!
I'm not sure this fixes the original problem, so leaving open
Yeah, module_builder_entity_type_build() expects to find it, because it dynamically adds further templates and needs a base URL.
Ru had some module which was crashing when config entities had a canonical template.
Try changing it to 'edit-form' in module_builder_entity_type_build(), since that has the same path.
Are you using the same module that Ru is which complains about canonical templates?
I can't reproduce your problem.
Yup, this module works well with Facets made with exposed filters.
Potentially? You'd need your filters to be made with a view.
I considered doing expanding fieldsets on a single overview page, but decided to follow the UI pattern on eBay and Vinted, of using subpages for each filter. I don't think supporting both UI patterns would make sense in a single module.
Breakpoint files are not config! They're YAML plugins.
Putting the whole seriali
> This is because prior to saving layout, the custom block entities exist in a limbo state, where they have no ID and thus cannot be loaded from entity. This breaks the entity load in displayView().
I've run into this problem elsewhere. There are two options:
- bypass the lazy builder if there is no entity ID and render directly. We know that we're not in a main rendering situation, so the performance hit is fine
- don't render anything if there is no entity ID
> Because changing node.settings.use_admin_theme requires a route rebuild
Let's add a comment to say that. Otherwise someone in the future will wonder about that all over again.
The big problem with converting form/render arrays to objects is BC. There's tons of form building code and form altering code out there which expects arrays, and passes them to PHP native array functions like array_merge() and so on.
If PHP have an Arrayable interface (like its Stringable) and if the array_foo() function accepted that, we'd be fine, but it doesn't.
I've thought of a way we could maybe do this, though it's not very pretty...
1. We pass a render object to form alter, wrapped in a catch{} block for a TypeError.
2. If we catch a TypeError, then it means something in the form alter chain is using array_foo() or something like that.
- 2.1 trigger a deprecation error
- 2.2. Convert the render object to an old-style render array, and pass it into form_alter again.
It would be:
- bad for performance
- you'd only get a deprecation error for the first use of an array_foo() function, and you'd have to clean that up before you saw the next one. Which would make it a PITA for updating your custom code, because you'd be seeing all the contrib ones you can't do anything about.
Looks good. Thanks!
Just a couple of minor things to change.
I don't understand the question, sorry.
It's been a while since I posted this, so I don't remember it much, but from my original post, the problem is this:
channels:
node:
- my_bundle <- %key of this is '0'.
channels:
node:
- my_bundle <- %parent.%key of this is 'channels'. Why is it not 'node'?
> public static function getCode(PackageInterface $root_package, InstalledRepositoryInterface $repository): string {
We'll need to add a parameter to that when 🐛 Wrong DRUPAL_ROOT with non-standard code structure Needs review joins in to add its own data to the DrupalInstalled class, probably the ManageOptions object. But that's fine to happen in that other issue, as the class with this method is marked as @internal.
> scaffold does not run if you install Drupal from core git repo
How do we resolve this?
Wrong project.
Note that if you're just using a flag as a UI to trigger the ECA action and you don't care about the flagging entity that doing that creates, you'd be better off using https://www.drupal.org/project/action_link → instead.
This is absolutely still relevant. There is no proper documentation on plugin types the way that there is for hooks.
Nobody has worked on it because it's a big undertaking.
return t("@controller controlling @dependents", [
'@controller' => $controller_filter,
'@controller' => $controller_filter ?: '',
There's a follow-on here:
If '@controller' is an empty string, then the UI text makes no sense.
Fixed.
The IS should explain the consequences of this bug more clearly. For instance, top-level constraints are not validated with this method:
$violations = ConfigEntityAdapter::createFromEntity($config_entity)->validate();
Also, the CR needs some editing for style. It should explain what is new, and how it has changed. The chatty style isn't appropriate.
It's been a long time since I wrote this module, so I don't remember how it all works.
But even entity types that have no bundles actually have a bundle -- it has the same name as the entity type ID. I think the change should be doing something with that.
This doesn't cover config entity validation which was added recently.
Have you looked at the README file in the module?
Also, there's no need to upload screenshots of a UI the maintainers already know about!
I think a plugin type for extracting files from a node is definitely the right way to go, as we have several issues where we need to expand how files are obtained:
- layout builder items -
✨
Make it possible to index attachments from index only fields that don't exist on the entity
Active
- nested references such as paragraphs:
✨
Impossible to index attachment in nested entity reference
Needs review
There's a couple of important changes I'd say the MR here needs though:
1. It should be possible to have multiple plugins active. For example, you might want plugins from layout builder AND from files referenced in body text tags.
2. Some plugins need to be recursive, and run enabled plugins on what they find. For example, you might want the paragraphs plugin to then run the body text plugin on the body fields on the referenced paragraph entities.
Sorry, it was 2 years ago at a firm I'm no longer with.
My best guess is that I had some array processing or extracting process plugin twice in the same pipeline.
Could the MR branch be rebased rather than have 11.x merged please? It keeps the history easier to read.
> Now you got a number characters left. Half is for the first part of the table/alias name and the rest is for the last part of the table/alias name.
That might cut off words in the middle, which is going to make some ugly table names.
I think it's better if callers can specify which bits of the table name they want to preserve -- it's fine if that's bits at the start AND at the end.
> To make the change as useful as possible for the views module, the last part of the original name/alias is very important. For others it is the first part. Therefore my suggestion is to replace the 'middle' part
That's fair enough. But what's the algorithm for deciding what counts as the 'middle'?