That still relies on the site owner knowing that this needs doing, and understanding how to handle a crash like the one in the IS. It's still going to leave a lot of people confused.
Shouldn't Composer clear the apcu cache whenever the composer.lock hash changes?
> Document this by adding a deployment example script that includes creating this.
I'm not sure what that would look like.
> I'd like to see an example of what the value for the identifier might be if the update was not related to a core Drupal update.
Agreed, I think it would be worth explicitly saying that the value is arbitrary and that all that matters is that it is changed.
AFAICT (with no docs it's hard to be sure, hence that issue!!!) deployment_identifier only causes a container rebuild. The PHP file cache is something below that, isn't it?
That's great, thanks!
This looks great, except we both didn't spot that with that service removed, there is no longer any reason for this class to have any DI code at all -- create() and __construct() can be completely removed, since they do the same as the parent class's methods.
> Already in core — see this for ckeditor5.plugin.ckeditor5_language for example:
Yes, but that's only the values. It doesn't have the option labels.
I'm making progress on this.
> Already in core — see this for ckeditor5.plugin.ckeditor5_language for example:
Yes but that's only the option values. It doesn't define the option labels.
The labels are needed for decoupled UIs, and also for translation.
There are at least 2 separate issues here:
> The problem is that there needs to be logic to remove BEF from the view's configuration when BEF is no longer used on the view
> have all the plugins provided by this module implement Drupal\views\Plugin\DependentWithRemovalPluginInterface and its onDependencyRemoval() method
t() should not be used with variables.
You seem to have replaced $kernel wit $http_kernel everywhere, not just in the classes concerned in this issue, and also lots of places where it's actually the DrupalKernel!
This issue is only about middleware classes.
Also, please can you make a MR rather than a patch?
Re #26, this doesn't seem to be a problem at the checkbox level. This form works and passes validation:
$form['test-3293609'] = [
'#type' => 'checkbox',
'#title' => 'test 3293609',
'#default_value' => TRUE,
'#disabled' => TRUE,
'#required' => TRUE,
];
$form['submit'] = [
'#type' => 'submit',
'#value' => $this->t('Submit'),
];
This one with a checkboxes element fails validation:
$form['test-3293609'] = [
'#type' => 'checkboxes',
'#title' => 'test 3293609',
'#options' => [
'alpha' => 'Alpha',
'beta' => 'Beta',
],
'#default_value' => [
'alpha' => 'alpha',
],
'#required' => TRUE,
];
$form['test-3293609']['alpha']['#disabled'] = TRUE;
$form['submit'] = [
'#type' => 'submit',
'#value' => $this->t('Submit'),
];
Tests are green and @alexpott has made the changes from his review, so this is back to NR.
> I think this should be process pipeline instead of process definition because the pipeline consists of many process plugins.
No, it shouldn't be 'process pipeline'.
The definition of the term pipeline is this:
> Each step uses a single process plugin, and a chain of process plugins is called a pipeline.
In the MR, we don't mean a single pipeline, we mean the entire 'process' definition of the migration.
Done!
I've actually added two techniques for optimisation -
- pre-query, using LAG() and LEAD() as described above. This will scale best to very large result sets, but the query alteration might cause problems with complex queries (such as using GROUP BY, maybe -- I've not experimented)
- post-query, removing rows from the result that are not relevant. This is similar to the patches above, but crucially runs *before* the SQL Views query plugin loads the entities for each row. It will still not scale as well, as for very large result sets, the whole initial result is still loaded into PHP -- but it's probably fine up to several thousand rows.
Please review!
@jvbrian You've made a fork and it says there's a commit you've made, but I don't see anything on the feature branch?
> I think we should stop using the class entirely here
How does that work though?
RoleStorageInterface is covered by BC, so that means if you ask for the storage handler for the role entity, should should expect to get that interface. So we can't remove the class at this point, it has to stay until RoleStorageInterface is removed in 12.0.
Oops, didn't mean to change status.
Though I'm confused why the other two replacements of this hook are hooks, but this one is a class? What's the difference?
That's good, but we'd still be losing the discoverability of it in the documentation as an API.
With hooks, there is a @hooks topic that explains the concept and lists the API elements. With HookyClasses, you only learn of a particular class if you happen to stumble on it.
> The FileCache is only invalidated by plugin class file modified time, so Drupal cache clears do not affect its contents. That means that installing or uninstalling a module will not invalidate the FileCache.
We've seen at 🐛 Class "Doctrine\Deprecations\Deprecation" not found Active that any package update could require a clearing of the file cache.
If we were to update our requirements to say that the filecache needs to be cleared whenever modules or packages are updated, it would give us more flexibility here.
Should we update our requirements to say that the PHP file cache should be clearer whenever modules or Composer packages are updated?
This could happen again any time that a package decides to move a class.
It could also happen if a package uses 3rd-party attributes -- see ✨ Allow attribute-based plugins to discover supplemental attributes from other modules Active .
> Makes sense to move this to a hard-coded class instead of a hard-coded function name, I don't there's anything else we can do in install.
The problem with these hooky-classes, like this and ModulePascalServiceProvider is that unlike hooks, we have no standard for documenting them.
guignonv → credited joachim → .
joachim → created an issue.
> to switch the query plugin
I think this is actually possible in the pager plugin after all.
It's core/modules/locale/src/LocaleConfigManager.php that needs fixing.
I can get this to work by hacking in the Views SQL plugin and wrapping the returned query:
$query->addExpression('LAG(node_field_data.nid) over ()', 'lag_nid');
$query->addExpression('LEAD(node_field_data.nid) OVER ()', 'lead_nid');
$outer = $this->getConnection()
->select($query, 'inner');
$outer->fields('inner', []);
// $outer->condition('lag_nid', NID, '=');
// OR
// $outer->condition('lead_nid', NID, '=');
return $outer;
This POC shows that it's possible to wrap the query in another, and use the lag/lead expressions.
However, to use this we need:
- to switch the query plugin, and Views only allows base tables to specify a different query plugin. We really don't want to have to redefine EVERY table (and expect users to know to use a different base table). What we could do is override getPlugin() in DisplayPluginBase, which would mean we need a custom display. But having an 'Entity Pager' display type makes sense.
- we need the current entity ID to pass to the query as a condition. We could do the same technique with the route match, though doing it that deep in Views feels a bit wrong -- would be nicer to have it as a default argument plugin.
Instead of hacking things out in prerender -- when the entities have been loaded already, which is going to be one of the big causes of poor performance -- I wonder whether we can restrict the query.
The answer here using LAG and LEAD looks promising -- https://stackoverflow.com/questions/32036864/select-a-row-and-the-rows-e...
Committed, with a tweak to the minimum of 10.1.
Thanks!
BTW, remember to check which branch you're on -- you made your commits on the fork's 2.0.x branch!
Also, our minimum core requirement is about to change to 10.1, so we should use the twig filter for the ID -- see https://www.drupal.org/node/3363551 → .
Couldn't this be done by adding the 'Display a specified number of items' pager to your view, and controlling which items show with that?
Should this part be in the NAV element, since it's not links?
{% if display_count and current %}
{% block count %}
{% trans %}
<p class="entity-pager-item-count">{{ current }} of {{ count }}</p>
{% endtrans %}
{% endblock %}
{% endif %}
Closing in favour of 🐛 properties that support tokens should say in the UI Active .
There seem to be some unrelated changes in this patch.
> we can’t have a single view with different displays for those content types
AFAICT you can.
I just made a view of nodes, with 2 block displays. One block has no sort order, and the other sorts by title. I set both blocks to show on nodes, and I see two pagers, with different prev/next links and a different display count.
Could you explain more about what doesn't work?
Yeah, just tried it and you can do '[node:title] >'.
This would need to be for 2.0.x as that's where new features are going.
Also, the MR has some code style issues.
But, given that the prev/next links support tokens, is this actually needed? Couldn't you just use the entity label token?
Committed. Thanks!
Thanks!
I'll commit the D11 updates and make a new alpha release.
@andypost I got this error on 10.3
The problem is that the class has been moved:
> lib/Doctrine/Deprecations/Deprecation.php → src/Deprecation.php
I don't know why autoload hasn't caught up.
I got this on Drupal 10.3.x, when I upgraded drupal/csv_serialization from 4.0.0 => 4.0.1.
The problem appears to be doctrine/deprecations 1.1.4. I forced a downgrade of that to the previous version I had -- 1.1.3 -- and with no other package changes, the error stopped.
Should this not be major or critical?
DomDocument is still pretty verbose.
I'd love to see something inspired by perl's CGI (https://perldoc.perl.org/5.12.1/CGI#CREATING-STANDARD-HTML-ELEMENTS), which allows you to nest things:
$q->blockquote(
"Many years ago on the island of",
$q->a({href=>"http://crete.org/"},"Crete"),
"there lived a Minotaur named",
$q->strong("Fred."),
),
$q->hr;
Tagging.
Rebased.
Hmm on second thoughts, I can't get it to work :/
Pushed everything to the branch.
Thanks!
I started looking at this, and figured I'd add test coverage. I've got something working (in branch 3487785-cacheability-tests) -- but I need to do a bit of clean-up on it as I had to try several approaches to get caching to kick in!
Something for a follow-on: I think I've worked out how to get page caching working with this.
I'm working on 🐛 Cacheability is not respected when not using a lazy builder Active and I can get the test to fail (as expected) if I do this:
- enable page_cache module
- kill CommandLineOrUnsafeMethod, which prevents the page cache from handling the request.
Would be great to get a D11-compatible release!
Yup, there was a new release but the D11 compatibility issue has been waiting since March: 📌 Automated Drupal 11 compatibility fixes for entityreference_dragdrop Needs review .
In addition to #21, another way to do it is to define a computed field on the entity, which returns a NULL value. You can then set $entity->mycomputedfield->value and retrieve it later from the same entity object.
Yes.
The module https://www.drupal.org/project/date_recur_search_api → uses a computed field which Search API then uses to filter and sort by.
The computed value is there any time the field is accessed.
Shouldn't enum cases have docblocks rather than line comments, the same as class constants and properties do?
What's the performance cost?
\Composer\InstalledVersions::getVersion() includes a file with 2000 lines of code in a monster array.
The docs of the type need to be enough for the context. The docs on the type won't tell you what the type is being used FOR.
Silly example, but given this and no knowledge of Drupal patterns:
> public function access($object, ?AccountInterface $account = NULL, $return_as_object = FALSE) {
how do you know whether this is 'access FOR the account' or 'access TO the account'?
/**
* Calculates the permissions for an account within a given scope.
*/
public function calculatePermissions(AccountInterface $account, string $scope): RefinableCalculatedPermissionsInterface;
No, because what's $scope? What does it represent, what are its valid values?
A variable name is not enough.
And for omitting docs based on the type, that's only good enough if the class/interface/enum of the type itself has docs that are sufficient to tell you what that parameter is doing in the context where you currently are.
> By "documentation," I mean a list of available properties with their types and a brief description. This documentation may already exist somewhere, but you would need to look it up yourself. For objects with typed properties and methods, IDE can provide that information for you.
In both cases, people need to write the documentation.
Though I get your point that with named parameters or object properties, IDEs will more easily provide information. But it has to actually be there!
What's the benefit of using DOMElement as our base?
How do we handle BC, especially with alter hooks that expect to find arrays?
> hard to find documentation for available properties.
I'm not against this direction of movement at all, but that is a reason to **fix our documentation**, not to replace a system with a new system that will need just as much documentation. People have been saying for years that form/render arrays need documentation.
Could you rebase this so tests run again please?
> Did some digging, and found this in FieldFormatterBase::view():
Nice find!
I've fixed the broken tests -- could you rebase this please so the tests can run on it again?
Left a comment on the MR.
I'm not actually sure how we fix this, as the dual meaning of the term 'config prefix' probably spreads to other things. e.g. in the storage handler we have:
* The prefix consists of the config prefix from the entity type plus a dot
* for separating from the ID.
This bit will need changing too:
> In the entity type annotation
LGTM, though maintainer will confirm this is correct.
Looks good. Thanks!
This is a good start, but they're not fields.
Something like:
> Provides default values for menu link plugin definitions.
I've been informed on twitter by @chx that it's the docs which are wrong.
These need to be changed to say:
- it's optional
- what happens if you omit it
Hook documentation needs to be converted to OO as well before we do this.
joachim → created an issue.
I suspect this is long since fixed, as node type for instance uses this base class:
> class NodeType extends ConfigEntityBundleBase
Because of energy costs of generative AI and its role in worsening climate change, I consider its use to be unethical. I'm completely opposed to adding generative AI to any of my projects.