Update: I spent awhile pairing on this today with @star-szr, who helpfully volunteered to review together. Unfortunately we didn't make much headway, but I wanted to capture some of the work we did and possible leads to pursue.
First of all, we started from a WIP patch I'd rolled which looks up the cardinality of the computed token field, and then attempts to render the token $cardinality times, rendering the token repeatedly but switching out the $delta value. I'll try to distill that patch to its key points and push on an MR shortly.
This general approach seems like it ought to be viable, although we noted a tricky point from a usability standpoint: this assumes that the token will accept a delta (ie. points to a field that has multiple values), and silently fails if that's not the case. We couldn't see a way to introspect or otherwise infer whether a given token "accepts deltas" or even identify that a token refers to a field (so that we might look up its cardinality ourselves), which could allow a mechanism to throw an error. We alternately considered attempting to render the token without *clearing* the string, and then check if the rendered value matches the original, and treat *that* as the error condition. Another possibility might be to just provide a "debug mode" to the module such that a site-builder can troubleshoot their field configs by being able to see what Computed Token Field is doing (ie. show a message when computing a field, show its name, token and rendered value). At any rate, we set this aside for the time being, and honestly it may suffice to just have some good documentation about cardinality and multiple values.
In terms of the bug itself, what we're seeing with the patch attempting to handle multiple values is that the computed value we return seems to be empty (the tokens aren't rendering anything), and that empty value is not being handled well, resulting in a 500 error, where the stack trace indicates a failure in validating the form (in the case of an entityreference computed token field), or saving the field value (in the case of a simpler plain text field).
Tracing this in a debugger, we were able to see the token rendering empty values, and for awhile it seemed that it may be related to the fact that the content entity is new, and so the computed token referred to a value not in the database yet. However, reverting back to the original code it appears that the computed value renders correctly even on an initial save (we should doublecheck this more carefully).
To the extent that the module should handle empty values correctly, this is somewhat beside the point, but looking at the code flow in this context helped to illuminate the problem somewhat. I suspect the problem lies with the "shape" of the values returned changing with my patch applied. In the original code we return a string for a Text field, whereas now we return an array. This is probably a good change, but I think some of the logic further upstream doesn't account for it. We could see that we ended up in the parent ComputedFieldItemTrait's isEmpty() method, which clearly doesn't anticipate receiving an array of values. So possibly overriding that function to handle that better could help here.
On the other hand, I dug up some old links in the Computed Field module that suggest a possibly different direction. This docs page → describes how to handle multiple values in Computed Field, and indicates (for Drupal 8+) that the computed function will get called once for each $delta in its cardinality. this issue in Computed Field's queue → shows some of the reasoning and implementation for handling this in an unlimited value configuration. It's not clear to me whether we inherit this "call the computed function for each delta" behaviour from Computed Field, but if so, it might explain why the approach above isn't working, and if not, perhaps it's something to pursue. It'd certainly help to have the delta passed to our compute function, and just include that in the token to render.
Final note: just at the tail end of our debugging session, we noticed that there are 2 variations of token that includes the delta one is like [node:field_example:0], and the other is [node:field_example:0:value]. I'm not certain what the difference is, though my hunch is that in most cases we want the value variation. Although possibly this varies by field type? Another piece to consider.
That's all for now: I plan to roll my patch into an MR to share it here, and then set this aside for the moment. I intend to work on 📌 Implement localdev setup and DrupalCI test harness Active next, to make it easier to work on bugs like this.
spiderman → created an issue.
@dydave I've spoken with @mparker17 about this, and we agree we should move forward on this one, but would like to get ✨ Deprecate passing extra parameters to __construct() to prepare for 3.0.x Active merged first, so we can deprecate using extra parameters on constructors *before* we fork a 3.x branch, so we don't have to maintain those until another major version. I'm planning to review the MR on that issue in the next day or two, hopefully get it merged, and then we can proceed with this one:)
@sriharsha.uppuluri: thanks for the bug report, can you tell me what version of Core and what version of ECA you are using? This module was built against the now-older version 1.x branch of ECA.
Looking closer at the linked issue 📌 Gitlab CI: Broaden Tests coverage and require jobs to pass Active , I see that we aren't actually testing against 9.5, but rather PREVIOUS_MINOR and NEXT_MINOR, per these docs → . That's more in line with what @mparker17 was originally suggesting I believe, so just to say I think we should proceed with Matt's suggestion :)
I agree with the general thrust of @mparker17's proposal here, but I also agree that if we can easily test against the 3 most recently supported core versions (ie. include 9.5 at this point), that makes sense to me. Having said that, I haven't looked at the codebase for structure_sync with an eye to core compatibility lately, so if supporting 9.5 is harder for some reason, then we should consider whether it's worth it (I'd love to hear from folks who are somehow tied to 9.5, in that case!)
But again: if our tests can run (and pass!) easily against all three versions, then presumably supporting all three shouldn't be onerous? I'm not aware of huge breaking changes in core between 9.5, 10.4 and 11, but in my mind if the BC layer to support this becomes too much, that's the point where we cut a new major version branch, and carry on.
That's my 2 cents, curious to hear what others think :)
Ok confirmed this behaviour in my test environment (Workflow Save as Draft NOT installed). Steps to reproduce:
1. Add a multi-value text field (I only needed one) with unlimited values to your node type with a workflow attached.
2. Go to node/add/workflow-node
3. Enter a title, and a first text value in your text field.
4. Click "Add another item" and enter a second text value.
5. Click "Add another item" *again*, and enter a third text value.
6. Click Save.
If I leave out step 5, everything's fine, so it seems to be something about the *second* AJAX call that trips things up. I've no idea where to look from there, but hopefully that gives you a clue @johnv! Let us know if you need more information or can't reproduce- it feels like we're *very* close to resolving this cluster of bugs, and I'd love to see it done :)
@johnv, @dcoppel: I am now seeing the buggy behaviour you're describing! It came up in tests from my client project, where we have an instance that I think has the same conditions you're seeing. We have 2 separate "Add another item" steps on 2 different multi-value entityreference (autocomplete) fields on the form. When I comment out both steps in my test, the Workflow state is created normally. When I leave *one* of the "Add another item" steps in, the Workflow state is created normally.
However, if I have *two* "Add another item" steps, then things fail quietly. The form saves correctly in my case, because I'm also using Workflow Save as Draft module, but it appears no workflow transition has been created. This strikes me as connected to the custom labels going away on the second load, seems like maybe a subtle bug in the form builder/ajax callbacks or something, that's losing track of the form_state value somewhere along the way.
I'm going to see if I can reproduce this in my "clean" environment, just adding some simple fields to my existing "test workflow" node type. I'm guessing Workflow Save as Draft is not a factor, but I'll leave it out deliberately to try and narrow down the STR conditions.
@dqd: I can confirm this patch is still in active use and needed on a D10 project I'm working on. It looks like it no longer applies on the latest 4.x release, but the problem definitely still existed up to that point :)
Bah, too quick on the trigger. New patch *should* fix everything.
Another patch fix.
Coming back to this today after seeing 🐛 Drupal\Core\Field\WidgetBase::errorElement() Active appears to be resolved, I can confirm that testing in my clean environment, the behaviour I described originally doesn't happen anymore. I can create a node with one user, put it directly into the second state (see that transition get created correctly!), and then login with a second user, and push it forward to the second state (without saving twice!)
I'm also testing this in the context of the larger project I came from, and so far everything looks good. I'll report back here if anything goes awry, but for now I'm going to set this RTBC, and look forward to the upcoming 1.9 release!
I noticed a couple pieces missing in the original patch, attached is a newer version.
I noticed some bits were missing from the patch to make this work against the latest Workflow code. Attached is a new patch with interdiff showing added HookHandler bits etc.
Just weighing in here- I've used this project most recently for a client project where not only did their requirements call for ALL the items mentioned in comment #13, but also a more complex set of workflow states and transitions than could easily be represented using the core Workflows and Content Moderation modules. We needed a different workflow for each of 4 different content types, each with different states/transitions, access controls, custom labels on the transition buttons, etc.
Beyond that, our requirements called for a fair bit of integration with other modules. We knew we need a Rules-like component, for example, to do things like hide certain parts of the content form depending on workflow state, or trigger an email on workflow state transition that could include tokens referencing email addresses in field values on the node itself. After a fair bit of research, we concluded that Workflow and ECA were best suited for our needs, and even spawned off a handful of related modules to fill in some gaps.
My sense is that the core module is great if you want a straightforward publishing workflow that applies to all or most of your (small number of) content types. That being said, this evaluation was now 2 years ago, so it's very possible things have changed. Even so, I think Workflow is a very capable and valuable addition to the ecosystem, and would be glad to see it remain (although I agree it would be lovely to have the distinctions more clearly defined!)
I've been thinking about this issue with Workflow the last few weeks as I watch @johnv struggle to fix a host of interlocking bugs across the 1.8 release. The project I'm working on most recently *does* have tests that exercise the Workflow behaviour that we rely on, but unfortunately they're not really applicable in a general sense (they encode client-specific node types and fields, etc). That being said, I am a big fan of `behat` and behaviour-driven testing, especially when there isn't a lot of test coverage to begin with. I'm all for unit tests and such, but it can often be hugely valuable to just start with a test like:
Feature: A node can move through workflow states
In order to publish my node
As a Workflow module user
I need to move a node through workflow statesScenario: I can create a node in an initial workflow state
... steps to create a node of a fixture type with a workflow attached ...Scenario: I can transition a node to another workflow state
... steps to create a node, save it, and then transition to next workflow state ...
and build on it from there. This has the benefit of documenting the "happy path" use-cases of the module at the same time as catching regressions: if you do anything that interferes with creating a node and transitioning its workflow state, these 2 scenarios alone will catch it! Obviously a comprehensive set of unit tests will help identify *where* that failure arises, but at least it's a start.
Anyway, all of that to say: I've been doing a bunch of contrib maintenance work the last couple weeks, and am very keen to modernize the infrastructure for my various projects to leverage Gitlab CI to easily run the core tests as well as a basic suite of Behat tests. I'd be happy to collaborate on helping with a similar effort on Workflow module, if desired :)
@johnv: I'm happy to report that as of the latest dev-branch today, this bug has gone away, and the intermediate error is no longer. Thanks for your continued efforts! in terms of your questions, happy to give some feedback on the linked issues :)
@dcoppel: I don't see what you're describing in my "clean" environment, but I also don't have any file fields or the like, it's just a very simple setup for testing workflow. I tried quickly adding a filefield and a multi-value textfield, but can't reproduce what you describe. I can add both a new textfield value, upload a file, and save the new workflow state without problems. I'm guessing something else in your setup is complicating matters. fwiw, I'm about to bring this into the client project using Workflow, which is a much more complicated content model. I'll report back here if I see any issues there :)
Success! I'm pretty sure I got to the bottom of this, once I found that UserViewsData was providing the filter, and started tracing backward to the relationship that core entityreference fields end up getting in the ViewsData array. Eventually I landed on core_field_views_data() in views.views.inc (which looks like it's going away in D11?), and figured out that our computed token entity reference fields were not being handled by this core method (due to our "provider" not being "core", and a slight difference in the "target_type").
As such, I've pushed a patch that copy/pastes the same logic from core_field_views_data into our computed_token_field_views_data_alter() implementation, and it seems to do the trick. I'll do some further testing in situ, but it looks like this should do the tricky. Yay!
I've been puzzling over this for awhile today, and as best I can tell this is really a Views-specific thing that we're missing, to wire up the Computed Token entityreference fields into the views_data_alter $data array correctly. I've managed to narrow down the core behaviour somewhat, and I believe the core views.views.inc is the key piece that adds the "X referenced by Y" relationships for entity_reference fields, and figures out which data table to join in based on the target_type of the field. What I can't see is why our Computed Token Field's aren't being recognized and included in there- we're obviously missing something, but I'm not sure what it is.
Possible we need to manually "wire in" the Computed Token fields in our existing computed_token_field.views.inc, which we've done for datetime fields. But I don't really understand what the shape of the $data array we should provide is. Alternately, perhaps our FieldType plugin just needs to implement some Interface it's not currently, or have an extra bit added to its Entity annotation, and it'll Just Work(tm). Any insights welcome!
I've run across a concrete case of this issue recently, so I'll see if I can make this improvement.
The case is where the source field is configured to allow maximum 2 values (user references), and the computed token field is populated incorrectly. It seems to *always* put 2 values in (even if the source field only has 1), and always the *first* user referenced in the source field. This seems to point the way to handling this better, as it seems like there's some "awareness" of the multiple values, but we're not iterating over the source values or something.
spiderman → created an issue.
Fixed and released in 1.0.0-beta1.
@johnv: Just made it back to review this issue today, and see if it resolves the issue in our project. Unfortunately, I can confirm I'm seeing the same error on the latest dev- branch that @dcoppel reported above (this is in my clean test environment with a simple node type, 1 workflow with 3 states):
The website encountered an unexpected error. Try again later. TypeError: workflow_node_current_state(): Argument #1 ($entity) must be of type Drupal\Core\Entity\EntityInterface, null given, called in /var/www/html/web/modules/contrib/workflow/src/Entity/WorkflowTransition.php on line 854 in workflow_node_current_state() (line 420 of modules/contrib/workflow/workflow.module). Drupal\workflow\Entity\WorkflowTransition::getDefaultStateId() call_user_func() (Line: 442) Drupal\Core\Field\BaseFieldDefinition->getDefaultValue() (Line: 169) Drupal\Core\Field\FieldItemList->applyDefaultValue() (Line: 271) Drupal\Core\Entity\ContentEntityStorageBase->initFieldValues() (Line: 129) Drupal\Core\Entity\ContentEntityStorageBase->doCreate() (Line: 94) Drupal\Core\Entity\ContentEntityStorageBase->create() (Line: 1172) Drupal\Core\Entity\ContentEntityBase::create() (Line: 203) Drupal\workflow\Entity\WorkflowTransition::create() (Line: 292) Drupal\workflow\Plugin\Field\FieldType\WorkflowItem->getTransition() (Line: 298) Drupal\workflow\Entity\WorkflowTargetEntity::getTransition() (Line: 81) Drupal\workflow\Form\WorkflowTransitionForm::createInstance() (Line: 106) Drupal\workflow\Plugin\Field\FieldWidget\WorkflowDefaultWidget->formElement() (Line: 459)
Coming from 🐛 Workflow doesn't recognize state change first time Active I noticed a seemingly related error when simply creating a new node with a Workflow field. I note a change in the behaviour of the module, where the "Change State" widget (in my case, we use radio buttons) no longer populates an initial option value for the state. Instead those radio buttons are empty, and then saving the node results in the following error:
The website encountered an unexpected error. Try again later. TypeError: Drupal\Core\Field\WidgetBase::errorElement(): Argument #1 ($element) must be of type array, null given, called in /var/www/html/web/core/lib/Drupal/Core/Field/WidgetBase.php on line 580 in Drupal\Core\Field\WidgetBase->errorElement() (line 646 of core/lib/Drupal/Core/Field/WidgetBase.php). Drupal\Core\Field\WidgetBase->flagErrors() (Line: 277) Drupal\Core\Entity\Entity\EntityFormDisplay->flagWidgetsErrorsFromViolations() (Line: 268) Drupal\Core\Entity\ContentEntityForm->flagViolations() (Line: 214) Drupal\Core\Entity\ContentEntityForm->validateForm() call_user_func_array() (Line: 82) Drupal\Core\Form\FormValidator->executeValidateHandlers() (Line: 274) Drupal\Core\Form\FormValidator->doValidateForm() (Line: 118) Drupal\Core\Form\FormValidator->validateForm() (Line: 593) Drupal\Core\Form\FormBuilder->processForm() (Line: 326) Drupal\Core\Form\FormBuilder->buildForm() (Line: 73) Drupal\Core\Controller\FormController->getContentResult() call_user_func_array() (Line: 123) ... snip ...
I've also noticed that even if I do select a radio button option, but then save the form and the form validation fails for some reason, the option I'd selected is not retained, and the radio buttons are empty once again, so the problem persists. I can imagine this would be related to the OP symptoms, because the AJAX call when "Add a new item" will attempt to rebuild the form and thus likely trigger the same code path that clears (or never populates?) the state options. Hope that helps!
@johnv: Please don't misunderstand, I am not trying to rush the process, I think giving a bit of space for recent commits to land makes good sense! I was mainly trying to understand your thinking and validate my sense that you were moving toward a new release soon-ish :)
I am also very happy to be a bit of a squeaky wheel at just the moment when you had attention to give. Thanks again for your efforts on this!
I have been able to *mostly* validate that the latest dev-branch code seems to resolve the core problem I was seeing, where a state transition wouldn't be recognized the first time. I have been working on adapting our custom code and related modules (see 📌 Adapt to upcoming 1.9 release of Workflow Active , 📌 Adapt to upcoming 1.9 release of Workflow Active , and 📌 Adapt to upcoming 1.9 release of Workflow Active ) to integrate into my client project, and have *almost* got our tests passing once again.
Unfortunately, there is one key problem remaining. I'm not certain it's exactly related to this issue, so let me know if you want me to create a separate ticket for it. What I'm seeing (this is back in my "clean" environment now), is a change in the behaviour when creating a new node. Previously the radiobuttons on the Workflow "change state" widget would have the "first state" pre-selected. This meant that our test would simply hit "Save" without explicitly choosing an option, and everything was fine.
As of today, what I see instead is a) the radiobuttons have no option selected initially, and if I simply hit Save without selecting one, I get a 500 error, with the following message:
The website encountered an unexpected error. Try again later. TypeError: Drupal\Core\Field\WidgetBase::errorElement(): Argument #1 ($element) must be of type array, null given, called in /var/www/html/web/core/lib/Drupal/Core/Field/WidgetBase.php on line 580 in Drupal\Core\Field\WidgetBase->errorElement() (line 646 of core/lib/Drupal/Core/Field/WidgetBase.php). Drupal\Core\Field\WidgetBase->flagErrors() (Line: 277) Drupal\Core\Entity\Entity\EntityFormDisplay->flagWidgetsErrorsFromViolations() (Line: 268) Drupal\Core\Entity\ContentEntityForm->flagViolations() (Line: 214) Drupal\Core\Entity\ContentEntityForm->validateForm() call_user_func_array() (Line: 82) Drupal\Core\Form\FormValidator->executeValidateHandlers() (Line: 274) Drupal\Core\Form\FormValidator->doValidateForm() (Line: 118) Drupal\Core\Form\FormValidator->validateForm() (Line: 593) Drupal\Core\Form\FormBuilder->processForm() (Line: 326) Drupal\Core\Form\FormBuilder->buildForm() (Line: 73) Drupal\Core\Controller\FormController->getContentResult() call_user_func_array() (Line: 123) ... snip ...
So far it seems that if I select one of the options, everything proceeds normally. I would prefer the original behaviour be restored here, as I notice that it also seems fragile in conjunction with our Workflow Save as Draft module, because the form_state doesn't seem to retain the value when the form page reloads due to validation errors. Let me know what you think!
@johnv: I've managed to do some further testing on this today, and it looks like whatever I saw earlier has gone away. The custom labels are working properly in my test environment, but I also cannot reproduce the failure I was seeing previously. As such, I'm closing this, with apologies for the noise :)
spiderman → created an issue.
This patch has been operational for a few weeks now. I'm going to commit and cut another beta release to test against.
@johnv: I will take another look at this today or tomorrow. It's possible I only saw this in the context of my client project, in which case there may be other factors at play. I'll see if I can reproduce in a clean environment and report back :)
@johnv: Amazing! Thanks for the update, I'll look at this again today or tomorrow, and report back.
I have a question for you as well. It looks like you have been working toward a 1.9 release with this recent spike of effort. Since you indicated you are finished your work on this module, do you expect to cut a new release soon, or is there more to be done? Ideally I'd incorporate a new release into my client project, rather than tracking the dev- branch, but mostly it would just be helpful to know what you're planning :)
spiderman → created an issue.
spiderman → created an issue.
spiderman → created an issue.
spiderman → created an issue.
spiderman → created an issue.
re: comment #18 above- I had seen this too, and have a fix that I'll share. However, it looks to me like that is a separate issue from the one reported here, so I'll start a new one for it :)
@johnv: I am still trying to narrow this down, and actively tracking the dev- branch and all the various issues you are working on (again, thanks!)
I was able to successfully bring the latest code into my main project, but it did require a number of small fixes in a number of places. Most of them were external, and I'm tracking them as patches to other modules to adapt to the Workflow updates toward a 1.9 release. The others I will report in this queue, just trying to see if they already exist or if they are new.
I will note here that the biggest problem I've seen so far with the newest code is this erroneous "You tried to set a Workflow State, but the entity is not relevant. Please contact your system administrator. " error message that shows up when creating a node, and my impression is that it may be the result of your efforts over on 🐛 New transition is not properly created in 8.x-1.8 Active recently. I'm not certain, but connecting these dots in your mind may be helpful? :)
Final note: I'm *also* seeing the extraneous "select a value" option in my *radiobuttons* rendering of the widget, as mentioned over in this comment 🐛 TypeError : count(): Argument #1 ($value) must be of type Countable|array, null given dans _workflow_transition_form_get_action_buttons() Active on 🐛 TypeError : count(): Argument #1 ($value) must be of type Countable|array, null given dans _workflow_transition_form_get_action_buttons() Active . My cursory search to find where that option was coming from turned up nothing, but I can see a lot of TODOs in the code I did find that relates to building these options, so possibly this is just WIP :)
@johnv: Sounds good! I'm glad I wasn't missing something about this block, and we have a way forward :)
@johnv Thanks for all your efforts on this module lately, I see a ton of commits going on. I've just run through my test environment setup again, using the latest dev branch. I'm tentatively going to report that the key problem of the transition not being created is no longer happening (and the other blocking issues I was seeing even creating a node with workflow have gone away). I plan to investigate a little more thoroughly and bring this code back into my main project for further testing, but this definitely looks like progress!
With that being said, I do notice some issues when first creating a node now:
- The radio button options list has a spurious "Select an option" radio that shouldn't be there (and if I do select it, I get WSOD when saving)
- Selecting any actual state and saving results in the error "You tried to set a Workflow State, but the entity is not relevant. Please contact your system administrator. ", except that everything appears to have happened correctly.
- The Workflow tab doesn't show the (creation) -> State1 transition anymore (this may be related to the second one, and perhaps the first workflow transition isn't being created?)
@johnv: I can confirm that a fresh install with the latest dev branch will not complain about this optional config (unless I try to force the matter, but that's a different story). However, in futzing with this and thinking it through a bit more, I'm even more convinced that this is the wrong way to approach the problem of shipping a block in a contrib module.
The thing is, what ends up happening is we successfully import a config named block.block.workflowtransitionform which applies to *nothing*. It's never used. In order for a block config to be useful, it has to mention a theme and region. To do so, I still have to go to my theme's page and "Place block", which will in fact create a *new* config named block.block.MYTHEME_workflowtransitionform that represents the block config for my theme.
All of that being said, in the course of experimenting with this, I realized that I don't even understand what this block is supposed to be for. Even without having a "correct" theme-specific block config in my site, I still get the workflow transition field rendered by virtue of my form display config. If the block is intended to be an optional or alternate way of showing that form (perhaps in another context, or elsewhere on the node form page), then I would think it should be up to the site-builder to a) disable the field widget in the form display and b) place the block in the appropriate theme and region *which only they can know*. Does that make sense?
If there is a genuine need to install a block config when Workflow is installed, then I think we need to revisit the approach, and do something programmatically in a hook_install() helper routine that would a) determine what is the default theme, and b) create the block using Config API calls directly, rather than trying to import a yaml file.
I might be completely wrong here, but I really don't see what value this block config is adding to the module, and suggest we drop it entirely. Looking at the history of the file, I can see that it originated with the theme explicitly set to "bartik" which meant that it would only apply to sites using that theme (and again, to do what exactly?). Since it changed from that, it seems to have never been able to install successfully for anybody, so I don't think it'll be missed ;)
spiderman → created an issue.
Hi @johnv- I'm guessing you meant you think this problem is solved on the latest dev- branch, but unfortunately I'm not seeing any improvement. As of this morning, with a fresh install and very minimal configuration (my test environment described above), I get the following error when simply going to the `/node/add` page for a node with a Workflow attached:
The website encountered an unexpected error. Try again later.
Error: Call to a member function getTransition() on null in Drupal\workflow\Entity\WorkflowManager::executeTransitionsOfEntity() (line 159 of modules/contrib/workflow/src/Entity/WorkflowManager.php).
_workflow_execute_transitions() (Line: 22)
workflow_entity_insert()
call_user_func_array() (Line: 416)
Drupal\Core\Extension\ModuleHandler->Drupal\Core\Extension\{closure}() (Line: 395)
Drupal\Core\Extension\ModuleHandler->invokeAllWith() (Line: 415)
Drupal\Core\Extension\ModuleHandler->invokeAll() (Line: 217)
Drupal\Core\Entity\EntityStorageBase->invokeHook() (Line: 900)
Drupal\Core\Entity\ContentEntityStorageBase->invokeHook() (Line: 564)
Drupal\Core\Entity\EntityStorageBase->doPostSave() (Line: 781)
Drupal\Core\Entity\ContentEntityStorageBase->doPostSave() (Line: 489)
Drupal\Core\Entity\EntityStorageBase->save() (Line: 806)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (Line: 354)
Drupal\Core\Entity\EntityBase->save() (Line: 277)
Drupal\node\NodeForm->save()
call_user_func_array() (Line: 129)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers() (Line: 67)
Drupal\Core\Form\FormSubmitter->doSubmitForm() (Line: 597)
Drupal\Core\Form\FormBuilder->processForm() (Line: 326)
Drupal\Core\Form\FormBuilder->buildForm() (Line: 73)
Drupal\Core\Controller\FormController->getContentResult()
call_user_func_array() (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 638)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 181)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 53)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle() (Line: 32)
Drupal\big_pipe\StackMiddleware\ContentLength->handle() (Line: 116)
Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 90)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 741)
Drupal\Core\DrupalKernel->handle() (Line: 19)
@johnv: I think we should reopen this bug, as I don't believe it's completely fixed. I'm running a site-install on a plain vanilla site (D10.4) and the latest dev-1.x branch from git. We have some config we import after the install, and when I do this step, it apparently is still trying to install this block config and failing. I get the same The <em class="placeholder">block.block.workflowtransitionform</em> configuration depends on the <em class="placeholder"></em> theme which will not be installed after import. in Drupal\Core\Config\ConfigImporter->validate() (line 823 of /var/www/*/app/deploy/main/*/core/lib/Drupal/Core/Config/ConfigImporter.php). as mentioned above.
It's not clear to me what the *correct* way to ship a block config in a contrib module, since the theme and region keys can't be known ahead of time. It looks like core ships with some block configs that are specific to core themes, but I haven't found a good example of a contrib module that can't know what themes are installed.
@johnv I missed your intervening comments earlier, as I hadn't fully reloaded this page nor seen them in my email yet! Thanks for reading my steps carefully, you are correct there was a typo, which I've fixed in the IS now. There is definitely a state change, so a transition should be created. That said, I'll try pulling the dev branch into my test environment again today, and see where that leaves me.
Also, thanks for adding our add-on Workflow modules to the project page! These both came out of the current project I'm working on, to meet requirements we couldn't satisfy otherwise. Indeed, we are already using Conditional Fields, but couldn't get it to work exactly as desired, in part because our node forms use IEF to include nested entities, and CF can't target fields across entities. We also didn't want the JS behaviour of making the field required immediately upon selecting the new state, but only when the node had been submitted in the new state. Your question makes me think we should explicitly distinguish Workflow Required by State from CF on it's project page, though :)
@johnv Surprisingly, Workflow 1.8 seems relatively stable for us in the context of the project I'm working on. We are using the patch from 🐛 hook_update_8003 to 8.x-1.8 installs optional config and breaks my site Active , and haven't fully upgraded Drupal core (still on 10.3.2 atm), but aside from this odd behaviour it's done what we need from it.
That said, I spent a few hours yesterday trying to recreate the minimal use case in an isolated DDEV environment. I installed a fresh (10.4.3) Drupal site, with just Workflow module itself, and configured a node type, workflow, states, transitions, and roles as described in the issue summary. Unfortunately I can't seem to recreate the exact behaviour I'm seeing in our project, because I can't even submit the node on the second transition. It goes like this:
- Role1 user creates node, and puts it directly into State2
- Role2 edits node, tries to put it into State3
- On submitting the form, it POSTs, but the form validation fails and comes back with an error "This value should not be null."
As far as I can tell, this is due to the Workflow field's value not being picked up properly somewhere along the way. I haven't found the exact spot, but tracing it in my debugger it appears that by the time it gets to the core ValidReferenceConstraintValidator for the workflow field, it doesn't seem to have any value (or entity?) and thus concludes it's invalid and throws "This value should not be null."
I'm going to take another look and see if I can unlock this to get closer to the original behaviour, as there's a lot of config settings involved. I'm also going to try downgrading to Drupal 10.3.2 in my test environment, to see if that makes any difference. However, it seems increasingly possible this bug is actually within Workflow itself, so I'll continue to detail my efforts to get to the root of it, and maybe that'll dovetail with your efforts to clean things up on the dev branch.
I will also note that I tried applying the patch from 🐛 Warning : Undefined array key "to_sid" in WorkflowTransitionElement::copyFormValuesToTransition() line 356 when using Ajax button from other field Fixed and couldn't get one to apply, and when I updated to the -dev release, the site broke entirely. The fact that we don't seem to need this patch in our project gives me pause, but it looked potentially related so figured it was worth trying, esp since I'm working on slightly different core versions atm. I also notice the follow-up 🐛 TypeError: 'WidgetBase::errorElement(): Argument #1 ($element) must be of type array, null given, called in WidgetBase.php' Active but haven't tried applying that patch as well. Any guidance on which combination of things is likely to work best is greatly appreciated :)
I've committed this and included it in the 1.0.0-beta4 release.
I realized that the second commit for this fix was committed on the wrong branch, and consequently never included in the 1.0.0-beta3 release when this bug was fixed. I've since cherry-picked this commit over to 1.x, and have released 1.0.0-beta4 to include it.
spiderman → created an issue. See original summary → .
In testing this on our project, I realized that the patch fixed some logic that was duplicated elsewhere in the module. We check whether the workflow state has changed in the case where "Update workflow" is pressed so we can validate it *has* changed, but also when "Save as Draft" is pressed so we can validate is *hasn't* changed. I've refactored the patch to deduplicate the "has it changed" logic and just set the error appropriately in the 2 different places we're checking.
Found a bug in some cases where $entity isn't populated. Updating patch.
spiderman → created an issue.
spiderman → created an issue.
spiderman → created an issue.
I've tested the formnovalidate approach and it works beautifully. I've reverted the previous commit and will add this (simpler and more universal) solution instead.
Having *just* committed, pushed, tagged, and cut a new release with this *apparently* oh-so-simple vanilla Javascript solution (feeling very smug, I must say), my esteemed colleague @Ambient.Impact pointed out that we can probably just use [formnovalidate](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#formnov...) on the button we add, and let HTML5 take care of the rest. I'm grateful and properly embarrassed that I didn't explore a pure-HTML approach further, but here we are. I'm considering reverting my earlier commit if I can prove that this approach also works :)
spiderman → created an issue.
spiderman → created an issue.
spiderman → created an issue.
We tracked this down to a couple of issues:
- The structure of a select list field is slightly different, so we had to special-case the logic to inject the
$field['#required'] = TRUEin the right place. - The upstream Workflow module also changed the structure of a *workflow* field slightly, so the way we were targeting it in the States API section of the code had broken
I'll update the IS to describe next steps, but I have a test and a patch that works locally. I'll bump the version constraint on Workflow (not sure when this change was introduced, but I'll pin it to the latest for now), and cut a new release of this module.
Some notes from a quick bit of research. I'll dig into this more tomorrow.
* https://drupal.stackexchange.com/questions/317524/how-to-use-required-an... (mixed reviews about whether this does or doesn't work using core states API mechanism)
*
https://www.drupal.org/docs/drupal-apis/form-api/conditional-form-fields... →
The odd thing about this, is it *seems* like the States API shouldn't be the only thing at play here. Our `_wrbs_process_form_fields()` function sets $field['#required'] = TRUE; for the configured field when it determines that the entity in question is in fact in the required state.
spiderman → created an issue.
I'm going to close this, having merged the MR. I'm still suspicious that this functionality might not work properly if the ECA model is not evaluated in the context of the node whose workflow field value we want to evaluate, but I'll have to leave it for a concrete use-case around this. Currently this works for all cases I have.
I managed to get an extra function definition at the end of the plugin while futzing with merging another issue. The MR is up-to-date, and I will merge it soon and cut a fresh release, as both the recent bugfixes seem to be helpful and tricky to disentangle. For now, here's a fresh patch file without the extra function definition.
Attached is a patch representing the latest state of the MR, introducing context_repository and using that to look up the node route.
This has been working fine for awhile now, so I'm going to merge it.
spiderman → created an issue.
Hi @jurgenhaas, many thanks for these helpful comments. I realized as I was submitting the issue that we are on a quite old version right now, so I recognize this doesn't stand much chance of getting in. I've started looking into upgrading, but we're relying on the Classic Modeler as it stands, and I can't tell if it's still supported in the most recent versions yet (will investigate) and I don't want to convert all our models if I can help it.
Anyway, thanks for blowing my mind about the Set ECA log level action for more focused debugging! I think this will be super helpful, and may actually obviate the need for this patch. In any case, I'm happy to close this if it's only helpful to me, and ideally stop pulling the patch from my end shortly as well :)
The attached patch simply drops the clause which logs a message when a Lazy appliance check does NOT apply. This seems to be the primary source of the verbosity. There is a corresponding log message when the check DOES apply, which I've left intact, as it seems more obviously useful, and for my purpose doesn't seem to actually happen a lot.
spiderman → created an issue.
Attaching a patch of the latest state of the MR.
The issue fork is now updated with all of the tasks outlined in the issue summary completed. This seems to work nicely in terms of improving usability as well as functionality.
spiderman → created an issue.
@avpaderno: Thanks for taking a look at this- perhaps I was unclear, but the issue was not posts missing that I was expecting to be there, but that one (Dec 11) post not being formatted properly. That being said, I had watched the feed for an update for a couple hours on Wednesday, hoping that a subsequent refresh of the feed would fix the formatting (having reverted the change that triggered it on my end), but it didn't seem to be happening. At any rate, the drupal.org/planet view of that post now looks correct again. I'm guessing it had correct itself by the time you looked at it, and thus my original description probably didn't make much sense!
Thanks again, I'll close this ticket now as there's nothing further to do :)
spiderman → created an issue.