This doesn't seem to be fixed, sorry.
Each time I add a record, it's set to 'I’m volunteering my own time', despite my having saved something else as default previously.
> There are 11 proposed values for the value of that input in here #3542433: Usefull help message or clickable fill for type?, some of them match the values from the "Category" on Drupal.org and some of them don't.
Yup, I saw that issue. Lots of values there don't match to our issue categories.
For this issue here, I think we can just do:
bug issue -> 'fix'
task -> 'chore'
It doesn't sound like that issue is going to change the initial 'fix' / 'feat' part, so why can't this be done independently?
I think there are going to be a lot of commits mistakenly marked as 'feat' that shouldn't be, because people will just take the default!
It also appears that other modules can add their own keys:
$field_name = $entity->getEntityType()->getRevisionMetadataKey('workspace');
This also fixes 🐛 missing revision log message for entity created by a pull Active .
@kentr Your screenshot is how they SHOULD look without JS -- and as you say, it's ugly but usable. I might be wrong about the OL/UL, but it's a list of links, anyway.
My screenshot shows how they're broken in Claro.
Getting fixed as part of 🐛 revision handling not working when an entity exists already but wasn't imported Active .
I think the problem is rather than it's not actually a test base class -- the name is misleading and should be changed.
Only 3 test classes inherit from it, and it's to do with testing things that core modules do. Therefore the dependencies on user and node are fine.
So I'd say we fix this, and then file a follow-up to rename the class to something clearer and fix its misleading docs too (' * Base class for Plugin API unit tests.' !!!)
If you're adding those conditions to the query, then shouldn't these bits be removed further down?
if ($revision->hasTranslation($langcode) && $revision->getTranslation($langcode)->isRevisionTranslationAffected()) {
> ser should be added as a dependency to the test module calling it plugin_test
That won't work -- kernel tests don't install module dependencies.
We need node because:
> 'node' => EntityContextDefinition::fromEntityTypeId('node')->setLabel('Node'),
For future reference: in a preprocess hook, you can do:
$additional_cacheable_metadata = new CacheableMetadata();
$additional_cacheable_metadata->applyTo($variables);
Converted to attributes.
Setting to NR to trigger the bot. If the stupid message was because it was @covers I'm not going to be impressed with PHPStan.
> We gotta get that MR green before we fully review. There are still phpstan failures.
Right, but I don't know how to fix them. I don't understand what it's complaining about.
> For nodes, we already have a full, in-context preview functionality via the workspaces module
Workspaces and preview are not at all the same thing and they don't satisfy the same need and use case.
Preview is for 'Let me quickly read back what I'm writing and see what it's going to look like when I save it'.
Workspaces is for saving it and having other review it. It's much heavier. Writing a whole entity revision to the DB for just a 30 second read-through is huge overkill.
If our preview functionality is a 15-year-old hack, then the hack is what's the problem.
Fixed the cspell complaint. (Tried to install yarn locally and in doing so I've broken or removed node, I'm not sure which...)
What's the problem here:
> 16 @covers value \Drupal\Core\Plugin\Discovery\LegacyDiscoveryDecorator.
> references an invalid class or function.
Well then we shouldn't have this:
> protected $subdir;
Do I need to fix the class names and var names, or just comments?
> Or is the review status just about general approach?
No, I was hoping this was ready -- I'd fixed all the errors I could see in the CI. The cspell one was red but the test result just looked like CI failures.
13136 is the right one.
11020 is a draft, which is more specifically about recursive subdirs, and as such should probably be on 📌 Deprecate recursive plugin discovery Active instead AFAICT.
Changed the deprecation message to be built from smaller pieces.
Added a test. I've given the test module & test class generic names so future deprecations to do with plugin discovery can be added to them too.
It also needs input from people who are interested in 📌 Make Connection::expandArguments() public to allow array argument expansion outside of Connection::query() Needs work , in case it's part of the API they would like.
Downgrading.
Would be a nice fix to have anyway.
The other views handlers need checking for this sort of things too.
Oh wait!
I get it. It's because we've just done a DB import, but not dropped the {entity_import_status} table first!
So it has a log for a node that doesn't exist yet, or even at all!
It's entirely custom, but inside BatchAPI operations callbacks, so they're returning that they're incomplete and letting BatchAPI handle the queueing.
I'm making good progress with this!
Though writing database handling for closure tables has been fiddly! Much better than my earlier approach though!
It no longer crashes - there's a form validation error.
But I've added disabling of the radios for existing modules + a description with a link.
I just filed 📌 remove return value from expandArguments() Active because core doesn't use the return value.
Would it be useful for other callers?
There is callback_allowed_values_function() in api docs, and various methods are marked as this.
If any are missing this, it's a specific docs issue rather than this global one, which I think we can say is done.
mailer module in core is just about sending emails, not at all about configuring / translating / altering / etc?
If so, its name should reflect that it's low-level.
What about using symfony_mailer, since the project page for that says it'll be retired after D11?
@quietone I am getting really frustrated with issues that are patently still relevant getting this stale issue copy-paste comment posted on them.
🐛 Wrong DRUPAL_ROOT with non-standard code structure Needs work might get in soon, which will provide a way to get the app root.
Mail module is my project for a new config-based mail system in core.
I've not managed to get any traction for getting it into core, but it's not abandoned.
Yes, OBVIOUSLY. None of this has been done.
> I think that could be because you're building the message in a variable, then putting that entire variable in the trigger_error(), and maybe the phpcs rule just doesn't pick that up at all?
No, because when I had the whole thing as a variable in an earlier version of my MR -- trigger_error($message) -- it was complaining with the Drupal.Semantics.FunctionTriggerError.TriggerErrorTextLayoutRelaxed sniff for the whole message.
I'll add ignores for the specifics -- this way we still get phpcs coverage for the overall format of the message.
Yeah, some of those are because my code is WIP.... but things like this
> 52 | WARNING | [ ] The url '{$this->changeRecordUrl}.' does not match the standard: http(s)://www.drupal.org/node/n or
are because it's using variables to build the message.
It looks like it's still going to need some phpcs ignores for the specifics, even if it's happy with the format -- result on my local:
29 | ERROR | [ ] Doc comment for parameter $deprecationMessage does not match actual variable name $changeRecordUrl
| | (Drupal.Commenting.FunctionComment.ParamNameNoMatch)
52 | WARNING | [ ] The deprecation-version '{$this->deprecationVersion}' does not match the lower-case machine-name standard:
| | drupal:n.n.n or project:n.x-n.n or project:n.x-n.n-label[n] or project:n.n.n or project:n.n.n-label[n]
| | (Drupal.Semantics.FunctionTriggerError.TriggerErrorVersion)
52 | WARNING | [ ] The removal-version '{$this->removedVersion}' does not match the lower-case machine-name standard: drupal:n.n.n or
| | project:n.x-n.n or project:n.x-n.n-label[n] or project:n.n.n or project:n.n.n-label[n]
| | (Drupal.Semantics.FunctionTriggerError.TriggerErrorVersion)
52 | WARNING | [ ] The url '{$this->changeRecordUrl}.' does not match the standard: http(s)://www.drupal.org/node/n or
| | http(s)://www.drupal.org/project/aaa/issues/n (Drupal.Semantics.FunctionTriggerError.TriggerErrorSeeUrlFormat)<
/code>
for this code:
<code>
@trigger_error("Plugin discovery of plugin {$plugin_id} {$this->deprecatedDiscoveryDetails} is deprecated in {$this->deprecationVersion} and is removed in {$this->removedVersion}. {$this->updateDetails} See {$this->changeRecordUrl}.", E_USER_DEPRECATED);
Tweaked the release notes -- render element, not field.
I was halfway through writing this when you posted your comment, so I'm going to post it anyway, then consider your points above/
I'm definitely leaning against a queue, because I think we need to store hierarchy data.
I've had a brief look at various ways to store hierarchy data in SQL. Unlike most of the examples I see, we have a need for multiple parentage, since one entity can have multiple dependencies, and one dependency can have multiple parents (consider two nodes which both use the same paragraph library item, or two nodes which both have the same other taxonomy term). We can also have circular references (two nodes which link to each other in the body text).
We don't really have performance requirements, as we're not aiming for speed. We would like it to be fairly simple to change the graph, as while we gather dependencies we'll be constantly changing the tree structure.
A closure table seems like the best thing for this (see https://www.slideshare.net/slideshow/models-for-hierarchical-data/4179181), with a column in the closure table for the distance.
Even without any circularity though, ordering is going to be a complex thing because of the the multiple hierarchy:
e.g. A requests A1, A2. A1 requests A1A, A1B. A1A requests C. A2 also requests C. How do we know when to do C, A1A, A2?
And potential circularity throws this all up in the air anyway.
I'm thinking we need to do a *decent* attempt at ordering the tree -- if A is our initial entity, we order its dependencies by their distance from A, in the hope that will be good enough for getting dependencies in first.
But each time we save an entity, we check the tree for its OWN dependents, and check whether they are all saved already or not. If not, then we save this entity anyway, skipping the unsatisfied dependencies, and mark our metadata about it being 'draft'.
We then have a 4th phase, after ALL entities have at least been saved in draft, and therefore have a local ID, and then we re-save all the draft entities (without causing a 2nd revision ideally) so that their reference field values are finalised.
So the 4 phases are:
1. Use the pull request parameters to populate a table of metadata about the entities to pull, 1 row per entity. This takes care of expanding either an entire channel, or a list of UUIDs. This phase can store JSONAPI data in the metadata if it has it available, so it doesn't need to be fetched from the remote again.
2. work through this table, fetching the data if necessary. Add an item to the table for each dependency, and work those too until all dependencies are done. Store dependency hierarchy.
3. Using dependency hierarchy for a 'best guess' at order, save the data. If an entity doesn't have all its dependencies ready, mark it our metadata to say it's in draft.
4. Work through all the drafts and re-save them to get all the local IDs in place.
Each phase will need a batch API operation which will re-run until it deems itself to be complete.
Right -- I understand the circular problem there of not understanding things you don't know about!
I've made a parent meta issue -- feel free to comment there with things about entity keys you're aware of that need clarifying, and I can make further child issues.
I'm struggling to parse the use of preg_match() here, which seems really bizarre.
What happens if the regex fails, which it will AFAICT if there ISN'T a leading slash?
> As for the phpcs error I haven't looked closely, but as long as you pass in parts you should be able to get the right format.
I considered passing in parameters for $project_name, $removed_version, and $cr_url, but it seemed a lot more fiddly for both the code and the caller! Do you think it would be better to do that, just to satisfy PHPCS?
Thanks!
That doesn't look like a good place to put it though -- it looks like if you set something bad, it just silently DOESN'T save it!
if (!empty($login_path) && \preg_match('/^\/(.*)/', $login_path, $matches)) {
$this->set('login_path', $matches[1]);
}
There should either be
- something in the form that fails validation
- something that just fixes a bad value and then saves the fixed version
I'd also like an assert() in the preSave(), so that we don't accidentally mess this up in future tests.
Hmm yes, queue is maybe wrong, because we keep relying on order. What we actually have is a hierarchy of data dependency -- albeit one with multiple parents, and **potentially circularity too**.
I wonder if with a custom data table instead, we could store the hierarchy data in a field, and then rely on that for our ordering.
Thanks for your comment!
At first I didn't follow what you meant at all, as I was thinking, surely the gathering of dependencies and the fetching of data are completely inseparable, since you have to make the JSONAPI request to know what the dependencies are!
But then I had a lightbulb moment, and yes, I think doing it the way you suggest is going to be better than the route I was going down (which involved an ImportRequest object which would persist in the batch operation, and which would handle the recursion, and so could keep telling the current batch operation that it wasn't done yet).
What we need to do then is this: we have 2 or 3 lists of operations to work through:
1. optional, only if importing a channel rather than a list of UUIDs: populate a queue of 1 item per entity to import. So the batch operation works as it currently does: advances the pager and reports it's incomplete until the pager has reached the end. But instead of fetching entities, it merely puts each single entity into a list.
2. work the list of entities to import. For each entity, analyse dependencies. Any dependencies get added to the END of the current queue. Again, this is a single batch operation, which reports that it's incomplete while queue items remain.
3. work the list of entities to import, this time doing the actual processing and saving of data. BUT it has to work the list BACKWARDS -- doing the dependencies first.
This is where I'm not sure whether queues are the right thing here. Because we work the same list in both steps 2 and 3 -- almost?!? I'm wondering whether we'd be better off using a custom table, which we don't destroy when we take items from it, so that we can work the same items in both steps 2 and 3.
One consideration is dependencies we see several times. We don't need step 2 to process them multiple times, BUT if a 2nd dependent sees them, we need them to go after that. E.g. we have A, A1, A2, B and B reports A1 as a dependency also. Then we need the order to be: A, A2, B, A1 because step 3 needs A1 to be imported before both A *and* B are.
But if we naively do that, then we're potentially moving A1 to be BEFORE its own dependencies! Support had: A, A1, A1A, A2, B. If we move A1 to the end, then it's getting imported before A1A which is no good. Urgh, dependencies!
I wonder if the naive approach is just to REPROCESS A1's dependencies because it's now at the end of the queue, so we'd end up with: A, A2, B, A1, A1A. But that's going to cause repeat processing of A1's data!
This is still a problem in core's JSONAPI.
Agreed, but that's taking this issue to a much wider scope. It's best if documentation issues target single, small elements of the docs.
Feel free to make a meta-issue called 'Improve documentation for EntityType::$entity_keys', and child issues with specific tasks!
Both of those will need adding to the PHPStan ignore thingy, as neither of them can be fixed -- AFAICT __call() implementations don't have a return type, and the deprecation message needs to be a variable.
I've figured this out -- see https://git.drupalcode.org/project/entity_share/-/merge_requests/136/diffs
Basically, to deprecate a plugin subdir/namespace, the plugin manager needs to do:
use LegacySubdirPluginManagerTrait;
// in __construct():
$this->legacySubdir = 'Plugin/ClientAuthorization';
$this->deprecationMessage = "The plugin '%s' is in a deprecated subdirectory {$this->legacySubdir}. It should be moved to {$this->subdir}.";
The rest of the work is done in LegacySubdirPluginManagerTrait and a discovery decorator.
joachim → created an issue.
Merged to 4.
The problem in #4 is that the entity type in question doesn't have an edit form class specified:
* "form" = {
* "default" = "Drupal\ascend_resource\Form\ResourceForm",
* "delete" = "Drupal\Core\Entity\ContentEntityDeleteForm",
* "revision-delete" = \Drupal\Core\Entity\Form\RevisionDeleteForm::class,
* "revision-revert" = \Drupal\Core\Entity\Form\RevisionRevertForm::class,
* },
so this code doesn't kick in:
if ($edit_form = $entity_definition->getFormClass($route_mapper_plugin->getEditFormClass())) {
I'm not sure what the fix should be here --
a. should the route /resource/1/edit/simple request the form mode fmm_edit_*default* rather than fmm_edit_simple?
b. should the fmm_edit_simple form class get defined anyway?
Hard to know the intention when the code has to few comments!
This is an issue on Claro - could someone check whether it's the same on Gin? 🐛 dropdown buttons are broken with Javascript disabled Active
From Slack, on how to handle BC:
> sounds like that's essentially the reverse of https://www.drupal.org/project/drupal/issues/3490787 📌 Remove exception when accessing a non-existing field with ContentEntityInterface::get() Active and the only option we can think of there is an extra parameter that you have to pass to acknowledge that you want the exception and then that behavior becomes the default in the next major and the parameter is a no-op that can be removed again
This has not been implemented.
Looks good, but all the subclasses need this property to be changed to have an @inheritdoc now.
Thanks for the research :)
So it looks like we can simple remove the bool from the docs? If so, we can tag this as a Novice issue.
joachim → created an issue.
How are smart date field values output in JSONAPI?
Is something in Entity Share's code altering that structure as it gets processed? Can you debug to check?
If ES isn't modifying the data, then the problem is in Smart Date's unserialisation of the incoming JSONAPI data. Can you try saving smart date field data over JSONAPI, to eliminate ES from the equation?
Folks -- commenting out bits that don't work is a HACK, it's not something you post to a MR!!!
Key module is required for the tests -- it's in the module's composer.json's 'require-dev' section.
Arguably, we shouldn't be using it, at least not for general authentication tests -- we support storing credentials in the database so we should use that, to keep the tests simple and covering the least possible surface. If someone feels like working on that, then please do.
We probably would still need at least one integration test with Key module though.
The right way to do this would be to add a new import processor, which runs in the STAGE_PREPARE_IMPORTABLE_ENTITY_DATA stage, and removes the data for the specified fields from the incoming JSONAPI.
YES, still relevant!
I think what I would do is try to add support for Core 10/11 and Commerce 2/3.1 while still on the 8 branch. It's fine to have the support in the code, but say that we won't *actively* maintain support for Commerce 2 -- in other words, we'll commit patches, but that's it.
If that works, then great.
If we hit a problem that means we can't support both, then we need to make a new major.
> It would be great to be able to control the version of Commerce during the GitlabCI build phase of running tests - locally I've just been editing the composer.json file to switch between 2/3 for tests.
You should be able to specify the version of a dependency -- see https://project.pages.drupalcode.org/gitlab_templates/info/common/.
If you add a requirement for 'drupal/commerce ^3' there, while in the module's dependencies it's '^2 || ^3' then it'll resolve to 3.
> Using the Attribute here when on Commerce <=3.1.0 causes a WSOD because of #3542545,
I don't think we need a separate branch for that. Use a 'conflicts' for '>= 3.0.0 || < 3.1' and that should work.
@smustgrave for documentation-only issues, I think it's fine to ask for review of proposed text in a comment rather than go through the process of making an MR.
> Pass '#global' to
Nitpick, but I think this conveys that '#global' is an alternative to the string or array of tables. Whereas in fact, it's a magic table value -- as seen in the IS, you can mix it with real tables. So something like:
> The special table name '#global' can be used to [etc]