🇳🇱Netherlands @johnv

Never mind. I found the reason. Will be fixed in next commits.

I am stuck, having an Error message "This value should not be null." upon node creation.
You have no such experience?

I am working on D11.2 with latest Workflow version 2.0+dev, with WorkflowTransition::baseFieldDefinitions for from_sid and to_sid = 'list_string'.
(please run update. php)

Hi,
I am still working on your problem.
Unfortunately, I am each time distracted by other small bits&pieces that hurt my eyes.
This gave me the insight that the entity_save() (including $this->executeTransitionsOfEntity ) is done AFTER the node_save().
So, it must be possible to not ADD the entity_presave() - as in your patch, but to MOVE the same function from entity_insert() and entity_update() into presave().
I do recall having some issues with the correct behaviour of these hooks. But that in an old version.

Let me do some tests.

Closing this, as I guess this is fixed now.

johnv → created an issue.

Please test/check latest dev version, no that 📌 Support core change record template_preprocess_HOOK Active is implemented.
It also fixes an error I introduced in the preprocess() function.

The presave() hook is still not in. Instead of adding an extra save() of the Entity, I'm investigating the use of setEntityWorkflowField(), which is used already. We also update the ChangedTime field already.

  // Update targetEntity's itemList with the workflow field in two formats.
  $this->setEntityWorkflowField();
  $this->setEntityChangedTime();

johnv → created an issue.

johnv → created an issue.

The WorkflowOnComment still works. But it is possible to first create a WorkflowOnComment comment type, and then add this Comment type to a not-intended Content type. Therefore, '?->' was introduced in the code.
An error message is added for this case.

The patch is polluted with this usage of '?->' on many other places.

johnv → created an issue.

johnv → created an issue.

Sorry for the delay. I spent my time on other module, and more/other foundational errors.
I committed your function template_preprocess_workflow_transition patch, slightly modified. I guess you did this since it was activated without any transition present?
I still cannot reproduce calling this function. Also, '#workflow_transition' does not occur in another part of the code. Something must be lost.
The comments hint to 'comment', so I will now test the use case that Workflow is added to the Comments, not the Node. There, the value should be copied from the Comment to the Node.
Now I realize that that is the only use case for copying back values to the node. I never tested the option to copy the 'extra fields' back.

So, from you initial patch, only hook_entity_presave() remains open.

The remaining topic is handled here: 🐛 Incorrect workflow_state_formatter['#label_display'] Active

The patch does not work for me.
At least the init() is needed to have a list of valid values.
There might be a dependency on 📌 WorkflowTransition references WorkflowStates as a reference_item, not a list_string Active . At the moment, the baseFieldDefinitions of states are defined as 'list_string', not 'entity_reference'.

Thisis fixed somewhere else.

This might still be an issue in 1.8, and perhaps in 2.0,
but it should be resolved in current dev version and upcoming v2.1.
The latest code has massaged away the difference between 'new entity' or 'existing entity with new field'.

Your analysis is correct, but the patch does not fit the analysis.
The committed change fixes the root cause - it sets a decent value for uid upon creating a new Transition.

Please test latest dev version and report back.

Attached patch defines the 'from_sid' and 'to_sid' state fields of WorkflowTransition as 'entity_reference', by subclassing that as 'workflow_state_reference'.

Conceptually, this seems better, but the work is still not done (Views display), and the impact is too big, adding many code, to implement this.

Above commit redefines the state fields in the WorkflowTransition back to 'list_string' .

johnv → created an issue.

This reverts the commit from #3432411-5: Add WorkflowTransition::uid as widget field using EntityOwnerTrait → .
Not sure why that commit would give another result for $user determination.

This should now be fixed. Please test latest dev and report back.

This is indeed a difficult problem.
The following commits are first cleaning the code, then a fix is added.
The result is that yesterday's season is still displayed. This is needed for e.g. nightclubs, where openingtimes are through midnight.
In the future, that season should not be displayed anymore of hours are not after midnight.
But for now, first remove the errors.

The second commit was needed because both 'short' and 'long' are valid options for 'time'/'weekday' and 'date'.
And Exceptions need both:
- weekday on widget,
- date on formatter.
So I needed to break this apart, and move the 'weekday' to Slot.php, since that class is bey definition in the widget, and the value is used per slot, not per day, as in the formatter.

Nice.
" It still shows midnight (even if there are hours for the exception) " Can you share a screenprint of this?

Indeed. Above patch should do the trick.

This should not be possible.
I do not understand how you have empty 'exceptions' settings.
But in any case, the 'defaultSettings' are there to prevent such cases.

Attached patch should fix the problem, not the symptom. The current code is too cautious.
Pleas test and report back.

johnv → created an issue.

Also, in my test system, the extra fields are added normally to the saved transitions, and can be recalled in the Workflow History view.

The are not copied back to the TargetEntity, and IIRC that was never the design.

There is also an otion to add Workflow to Comments, and there, the comment field must be identical to the TargetEntity field. I need to test that scenario.

I do not understand the following:

diff --git a/workflow.theme.inc b/workflow.theme.inc
index 6a7da29..e78d4dd 100644
--- a/workflow.theme.inc
+++ b/workflow.theme.inc
@@ -19,8 +19,17 @@ use Drupal\Core\Template\Attribute;
  *     the details element. Properties used: #children.
  */
 function template_preprocess_workflow_transition(array &$variables) {
+  $elements = $variables['elements'] ?? NULL;
+  if (empty($elements)) {
+    return;
+  }
+
   /** @var \Drupal\workflow\Entity\WorkflowTransitionInterface $workflowTransition */
-  $workflowTransition = $variables['elements']['#workflow_transition'];
+  $workflowTransition = $elements['#workflow_transition'] ?? NULL;
+
+  if ($workflowTransition) {
+    return;
+  }
 
   $variables['from_state'] = $workflowTransition->getFromState() ?? '';
   $variables['to_state'] = $workflowTransition->getToState() ?? '';

- In my D11 system, the hook is not even called.
- why + if ($workflowTransition) , should this not be : + if (!$workflowTransition) { to avoid an error in the following lines?

this part is committed in the latest commit for another issue , by accident:
I think I remember removing it. It was a mistake.

diff --git a/src/Form/WorkflowTransitionForm.php b/src/Form/WorkflowTransitionForm.php
index 9ae3804..2d4f15d 100644
--- a/src/Form/WorkflowTransitionForm.php
+++ b/src/Form/WorkflowTransitionForm.php
@@ -154,6 +154,11 @@ class WorkflowTransitionForm extends ContentEntityForm {
     // since it will be done in many form_id's.
     // Keep aligned: workflow_form_alter(), WorkflowTransitionForm::actions().
     // addActionButtons($form, $form_state);
+
+    if (!empty($actions['submit']['#value'])) {
+      $actions['submit']['#value'] = $this->t('Update workflow');
+    }
+   
     return $actions;
   }

johnv → created an issue.

Ah indeed, updating OP.

In my test case, both getName() and id() exist, but give differente results.
- edit node with office_hours.

What is your use case/ test? perhaps WebformOfficeHours?

Which drupal version are you using?
I am sure this is yested.

johnv → created an issue.

johnv → created an issue.

johnv → created an issue.

johnv → created an issue.

Hi rahulkhandelwal1990,
This issue is about anonymous users.
We still have an open issue for authenticated users 🐛 Status field caching still broken Active
Anw the two of us have an open discussionin ✨ Rework displaying dynamic information to JS for persistent caching Active
Let's discuss over there.

Thanks a lot.
I did see the broken test after 'improving' the code base, but would not know how to fix this.

Please upgrade to new version 2.0.0.

Oh, I am working on 8.3 I think I must go back to 8.2, because I get more such problems reported.

Thanks, added.

The above commit should solve the problem.
Please test and report back. Then I will release a new version.

Indeed, I see many more problems:
- no problem with the 'Add exception' buttons
- both fields have the same title
- when first adding an exception one second field, then save, then add exception on other field, the value of first field is prefilled.

Regarding your remark #3.
I did change the $transition->to_sid and $transition->from_sid baseFields from 'entity_reference' to 'list_string'.
'list_string' does not need ->setSetting('target_type', 'workflow_state') .
ITMT, also because of your remark, I returned to the original, better 'entity_reference'

johnv → created an issue.

This should do it. Doing a string conversion of the 'options' already in module code, instead of relying on core.
Strange why a select option is treated different then a button title.

Off-topic: Sowieso, it is a crime to do the conversion between selectlist, radios, action buttons, and dropbuttons, espacially when using them in 'operations'.

Oh, I have it in my system, too.

Can it be a problem in the theme? My core themes give no problem. (since there is a lot of Twig calling involved)

Hmm, is this in module code on a node edit form with 'Workflow Transition form' field widget?

ITMT, upon your next module update, please run update.php and test the history view.

ITMT, upon your next module update, please run update.php and test the history view.

I realize this issue discussed more then one topic.
So, Indeed, it iseems best to close (and re-rename) this issue and regard only the original problem . Then the submit problem deserves a new issue.

If you don't mind, i will treat it as an edge case and a core issue for now. And will focus on getting a release 2.0.0 out.

Renaming the issue.

Thanks, it should now be corrected.

In FormBuilder::doBuildForm(), the following happens:

      // If a form contains a single textfield, and the ENTER key is pressed
      // within it, Internet Explorer submits the form with no POST data
      // identifying any submit button. Other browsers submit POST data as
      // though the user clicked the first button. Therefore, to be as
      // consistent as we can be across browsers, if no 'triggering_element' has
      // been identified yet, default it to the first button.
      $buttons = $form_state->getButtons();
      if (!$form_state->isProgrammed() && !$form_state->getTriggeringElement() && !empty($buttons)) {
        $form_state->setTriggeringElement($buttons[0]);
      }

      $triggering_element = $form_state->getTriggeringElement();

Working on this.
I have an unlimited txt field and an unlimited file field.
Adding multiple texts, then save, indeed does not seem to save the initial transition.
In FormSubmitter::ExecuteSubmitHandlers(), the handels is "file_managed_file_submit", which is not used at all in my case.

If you use a Views display, the from_state and to_state fields must be deleted and added again, since the key is changed from 'target_id' to 'value'.
(but let me test)

The link is a 'list_string', in order to get proper widgets for this.

I might revert to entity_reference, but that will get 'autocomplete' widgets, too, in the workflow settings.

That seems like an oversight. Let me check.

Please try new dev version. the change might help.

I experienced the same thing on testing.
I found out that the weights on the workflow setting, stages list were identical.
Only if i dragged a state, the system renumbered the staties.
I assume the weights in your yml file are copied to the workflow states setting page.
But please check.

Regarding "- In v1.8, you could use workflow on an entity before saving it. In v1.9, the entity must be saved first."

I did notice this in the last week, and removed the code (thinking it was superfluous, and not the task of a transition to save the node), but did not realize it was a feature, too.
It could be reverted.

I would love to receive some help with that.
As a part time developer, the D8, 9, 10,11 changes require a lot of effort to keep up avoiding technical debt...

Did you run update.php.
Version 1.8 and 1.9 are about better usage of baseFieldDefinitions.
This also includes from state and to state.
If update.php does not work, you should remove and add the fields to the view.

IN #7:
- The function workflow_get_workflow_state_names() is missing in v1.9.
--> I can add this back
- The required parameters for WorkflowTransition::create() have changed a lot. e.g. without a wid value, no transition is created.
--> That should not be the case. I added the 'entity' as an input paramater, since that holds the $sid and $wid. Also $state holds $wid.
- In v1.8, you could use workflow on an entity before saving it. In v1.9, the entity must be saved first. If your code relies on the old behavior, you’ll need to adjust your logic.
--> Ofcourse, my test situation may be less complicated that yours.
- In v1.9, you must set an initial workflow status when creating a node. This wasn’t required in v1.8.
--> Before 1.8 upon node create, the widget has an initial value, being the 'first' state. it was removed shortly in dev, but not in 1.8. In current version, the 'first' state is set automatically. your contrib code should not be worrying about that.
- Some interfaces are missing.
--> Indeed, I introduced WorkflowTargetEntity and WorkflowRole, at some point, Roel and Entity should be subclassed/decorated.
- For some reason, I now sometimes get the workflow status as a system name instead of a label/value. When I switch back to v1.8, this problem goes away.
--> Is this only on the creation formatter? or also in other situations. I should have fixed this 2 weeks ago.

In #4, you should be able to use the standard 'Value' formatter, instead.
Indeed, I do see that the 'creation' state returns a machine_name, not a description. Not sure why.

I have added your proposed changes from #8.
Indeed, there are multiple changes in the code and interfaces.
Version 1.8 was an attempt to change the code to use more core code, i.c. using baseField definitions.
There are reports that this is crippled, also when using hooks.
And some changes (e.g., WorkflowManager functions moved to WorkflowTargetEntity) are indeed just moving code, for better DX in the long term.
I will need to take a look at your 3rd paragraph in #8 separately.
When creating a WorkflowTransition, it should still not be needed to have a to-state. In the past, the to-state could not be defined, in the new version, the to-state is defined (either first state when entity->isNew(), or else same state). Then, It can be overwritten by the widget.
Your change in the $this->entity_id might make a difference, since new Nodes have no ID, yet.

Indeed,
several interfaces have changed in v1.8 (which should not be used) and upcoming v1.9.
This is due to the implemenation of the baseFieldDefinitions of Workflow(Config)Transition.
I did not anticipate uon usage of function like above by third party modules.

(It seems you code can be replaced by workflow_state_allowed_values() )

Hmm,
you are using an outdated version.
I am about to release v1.9.
v1.8 is crippled (as you can see from some issues).

Please update to latest dev and report back. I will wait for your response, and then create a new -working- version

Also, please take a look at ✨ Rework displaying dynamic information to JS for persistent caching Active . Perhaps we can take this a step further.

Indeed,
that hook does not do your trick.
IIRC I never got reaction from the requester after implementing it.

I guess we hve 2 problems at hand:
1- the time is not properly formatted, according to timezone
2- the field cache is not correct for users from different time zones.

Let me then rephrase the title of this issue from:
"Rework displaying dynamic information to JS for persistent caching
to:
"Field cache and formatter incorrect when working with multiple timezones

ad 1- time format
The following hooks are available:
- hook_office_hours_time_format_alter()
- hook_office_hours_current_time_alter()
I guess you need a:
- hook_office_hours_timeslot_alter()

At the short term I cannot help you out with this seemingly complicated issue.

I invite you to check out OfficeHoursItem::formatTimeSlot() and propose a fix in that function or a calling function.
Please also check the code by searching on 'timezone' and also the Issues →

It was determined that the timezone cannot be determined from within the module, but needs to be fed from custom code, since it is undefined where the appropriate timezone is set (user, entity, ...)

ad 2- field cache
As mentioned earlier, 'dropping the full page cache' is not the idea of the current implementation.
A lot of effort is put into a correct maxAge, after which , indeed, a full page load is triggered.
But for anonymous users, only the field itself is refreshed.

Please check OfficeHoursFormatterBase, lines 670-704.
There you can remove the checks for $this->currentUser->isAnonymous() and moduleExists('page_cache').
This influences the field caching and starts a JS statusUpdate for only the displayed field.
It normally only is activated for anonymous users, or when seasons and exceptions are in place.
(Bear in mind, that there is a check to 10 seconds in the JS, so debugging might influence the DX)

Also see 🐛 Status field caching still broken Active for an open issue, that I did not grasp entirely, but may be similar to your use case.

Please report back with your results.

It may be that we end up with a field setting 'always update the status, upon each field display'.

Thanks for all your help.
A small fix was added. A new release will be created soon.

No need for access, thank you.
The above should fix the problem.
The 'groupDays' messes around with the items. Not sure yet how that influences the other formatters, but moving the cloning of the items does the trick. Please check new version. I will release a stable release soon.

@artis, That is a nice page, BTW.
In my dev environment, I can only reproduce the error if the View does not only contains the normal 'Office Hours' formatter, but also the 'timeslot' field. There is no problem when adding the 'status' field, en did not test the 'season' field, yet. Probably, adding the normal formatter twice would generate the problem, too.
I did find how it happens, but am not sure yet why, or how to avoid it - working on that.

Your view does not have the 'timeslot' added. Do you have multiple office_hours fields in a resutl row?

OK, thanks, now i understand.
It was not clear to me that the problem happens in a Views display.
The hours are displayed correctly n a node list, but not in a Views Display. Strange...

Can you specify a test script?
Also, use latest dev.

Thanks fot your detailed reply.
I still cannot reproduce.have you Upgrade to v1. 26?

Nice! Does the error log report not give a message?