Actually, I just thought of a use-case for us where this would come in handy. And, not only does that #states ticket get us part of the way there, 🐛 content_moderation support Needs work introduces content moderation published states as well. And, yes, from there, I agree, some logic and a UI to handle state selection would get us the rest of the way. Looking forward to this.
Postponed until the above are adopted.
Rolls fresh patch from MR !27.
Ready for review.
jcandan → changed the visibility of the branch 3178100--content-moderation to hidden.
Original summary
Problem
The logic that has been added to the RequireOnPublishValidator for the paragraph entities is not correct. Paragraph can't get the parent entity since it doesn't exist when a user creates a new node. Also, even on the node edit, when a node exists and the paragraph can extract the parent entity, it will get the original node rather than a new one and therefore it will contain the old 'Published' value.
- Create a paragraph type with the 'Require on publish' option enabled.
- Add this paragraph to a node.
- Create a new entity with an unchecked 'Publish' option.
- Observe that 'Require on publish' validator returns an error for the required paragraph fields.
Proposed resolution
- Since at the moment, when a user submits the form validation constraint doesn't have the fresh node, we need or extract the published value from the POST data
- Skip the validation for the paragraph entities and when constraint function goes through all entity fields, find paragraphs fields, get paragraph entity and then go through all paragraph fields as well.
Okay. There is quite a bit going on here.
- The original issue summary steps to reproduce make no mention of content moderation.
- It turns out that he did fix the original issue as stated in #6.
- Others jumped in and suggested it was content moderation where it failed, which is true after #6 was merged.
- From thence, many have put forward several independent efforts to address this issue.
- Including my own duplicate issue noted above.
I will mark my issue as a duplicate, as well as
💬
Paragraphs integration not working with workflow moderation
Active
.
I will port my work from
🐛
Fix Paragraphs support with Content Moderation
Active
so I can give folks here credit for their contributions.
Once I get that ported over to a new MR associated with a fork here, please conduct reviews to ensure this addresses the issue well.
By the way, just to clarify why I think ✨ Add require on publish as a conditional field option Active may be related is that it introduces state handling for conditions (e.g. when some other field is check, make this one required on publish).
Though it may seem 🐛 Fix Paragraphs support with Content Moderation Active has more to do with this module's Paragraphs support, it adds support for moderation states that are considered Published.
I don't think we had this before.
$workflow = $this->moderationInfo->getWorkflowForEntity($parent);
$new_state = $workflow?->getTypePlugin()->getState($moderation_state[0]['state']);
if ($new_state) {
$is_published = $new_state->isPublishedState();
}
I don't think that fully encompasses this issue's desired outcome, but it may be a step in the right direction.
Do you still want to support Drupal 9? This might be worth making the breaking change for 🌱 Require on Publish 8.x-1.11 or 2.0.0 stable release plan Active .
Ya know! Actually, I missed this ticket when I created 🐛 Fix Paragraphs support with Content Moderation Active . Please take a moment to review the patch/MR there. Sorry if this ended up being a duplicate ticket!
Rolled MR !24 into a stable patch file.
Ya know! Actually, I missed this ticket when I created 🐛 Fix Paragraphs support with Content Moderation Active . Please take a moment to review the patch/MR there. Sorry for the duplicate effort!
My bad, I missed this ticket when I created 🐛 Fix Paragraphs support with Content Moderation Active . Please take a moment to review the patch/MR there. Cheers!
Obviously, if any tickets make the list that introduce breaking changes, going with 2.0.0 is appropriate. However, given the broad community adoption of the un-prefixed, semantic version, it may be prudent to use this opportunity to make the switch now to 2.0.0, even if no breaking changes are introduced. Just something to consider.
Title scope seems to duplicate ✨ Required on Workflow State Active , however a cursory review of the issue summary and discussion lead me to think not so.
Either way, I do wonder if there's any chance
✨
Add require on publish as a conditional field option
Active
fixes this. I haven't tested, but since it adopts #states
condition handling, that might address this issue.
Any chance
✨
Add require on publish as a conditional field option
Active
fixes this? I haven't tested, but since it adopts #states
condition handling, that might address this issue.
As per #13, updating the scope of this ticket to target the name field only. Address field already has a ticket in the queue: 📌 Add support for Address field Active .
Updated the issue summary based on understanding captured in ✨ Add support for multi-value fields like Name and Address fields Active .
Original report
Problem/Motivation
Address field is actually detected by 'Require on publish' module ('Warning on Empty' message is well displayed) but no indicator is added to the field label.
Might be useful for all other fields that output their widget #type as 'details'.
Remaining tasks
Add the missing 'form-required-on-publish' class to the <summary>
tag.
If 🐛 Fix tests failing due to unknown option verbose Active gets adopted, you'll need to re-opt-in for Drupal 11 testing.
See https://github.com/thefrosty/wp-upgrade-task-runner/pull/86 where the option was removed from PHPunit.
By adding full support for #states Form API conditions, integration with Conditional Fields → is possible with:
use Drupal\Core\Form\FormStateInterface;
/**
* Implements hook_conditionalFieldsStates_alter().
*
* @see Drupal\conditional_fields\Conditions::conditionalFieldsState
* @phpcs:disable Drupal.NamingConventions.ValidFunctionName.InvalidName
*/
function require_on_publish_conditional_conditionalFieldsStates_alter(array &$states) {
// phpcs:enable
// A list of supported states that may be applied to a conditional field.
$states['require_on_publish'] = t('Required on Publish');
}
Fork description
So that the module doesn't depend on Conditional Fields, the logic is provided in a submodule. When enabled, it provides require_on_publish
as one of the possible #states
.
Steps to reproduce
Using the provided fork, you may begin to understand how this might work.
- Install Require on Publish and Conditional Fields.
- Add a boolean field to Basic Page, and a text field.
- Under Structure > Conditional Fields, make the text field required on publish when the boolean field is checked.
- Create a Basic Page.
With the above and the branch as it stands, the required on publish markings are successfully applied. However, the error handling still needs work.
Problems persist
See comment above and comments in code for an understanding of the ongoing problems.
I've pushed a couple of commits to the fork (noting here in case someone overrides):
I am still working through some issues:
- I think I need to prevent error message removal when the condition is not met in
\Drupal\conditional_fields\ConditionalFieldsFormHelper::formValidate()
. - It seems the dependentValidate() thinks the condition is "triggered" when it is not.
- The
dependentValidate()
also seems to be explicitly handlingrequired
and!required
. - Not yet sure if it is best to pass the _wrapper in the error key.
Code standards fixes.
@Katy Jockelson → , I think I have good news. I was able to reproduce the issue. Did you have content moderation enabled for your content type with the RoP Paragraphs field? If so, it is being skipped outright.
I have reported it as a bug: 🐛 Fix Paragraphs support with Content Moderation Active . Follow there. Cheers!
Fixed issue where other field validation messages were being overridden.
When a simple field is required on publish, and the compound (e.g. name) field error validation is ordered before the simple field's messages, the messages are all replaced by the @field_label also requires the following parts when publishing: <em>@components</em>
.
Needs work: Incorrectly replacing other field validation error messages with @field_label also requires the following parts when publishing: <em>@components</em>
.
Patch #4 successfully applied and resolved the issue.
@Katy Jockelson → , I am unable to reproduce the issue as described. I tried a vanilla Drupal instance. Created a simple Paragraph type with one field required on publish and the other not. I added that Paragraph field to Basic Page. The markings and validation worked as expected. Drupal 10.4.3. Require on Publish 8.x-1.10.
Even though features like embedding timelines are working as expected here https://publish.twitter.com/ with the same principle.
Seems to no longer be the case:
This makes sense. The ordered list number 4 needs to be updated to 3.
@plopesc, thanks for your work on this.
Your test correctly failed prior to the fix, and passes after the fix. I've refactored a bit, removing the AJAX adjustments and adjusted the conditional logic for persisting the country selection. Feel free to add an issue if you'd like to describe steps to reproduce the AJAX issue your team is experiencing.
MR !14 is ready for review. Will leave this open for a couple days to receive feedback, but this looks good to me.
I'd imagine that, until this is added to Radix, in order to adopt
Storybook →
, we'd just have to create our own *.stories.twig
files in our sub-theme, and for those, a *.compentent.yml
with the replaces
key pointing to the target component in Radix. Anything else I may be missing?
Added ✨ Bump supported intl-tel-input version Active . Seems appropriate to get the front-end package dependency a good solid update before a major release.
When 2.0.0-rc2
gets released, it will introduce breaking changes (changes that require consuming applications to make code modifications to continue working correctly with the new version). While release candidates may be considered close to stable, this update is long overdue, and introducing breaking changes on a release candidate isn't strictly forbidden. We'll be sure to communicate this in the release documentation for RC2.
@plopesc pointed out that this feature may already be addressed by Telephone Validation → . I wonder if @omkar-pd might be willing to share reprucible steps if this becomes an issue with the modules not playing nicely.
Removed 🐛 Fix validation not being applied Active as a release blocker. Willing to consider putting it back in. Awaiting community feedback.
I realized that this should be marked as a feature request, not a bug. To fail validation and prevent save might even be considered a breaking change. Though, if the community wants this, we could add it to a release candidate before making it to 2.0.0. Perhaps even make it configurable.
Moving this out of 📌 Release 2.0.0 Needs review , unless someone objects.
In your video, you type the exit code and country code into the field, rather than select the flag. I was able to reproduce this bug by selecting the flag.
I was able to reproduce this issue. Digging.
Need to verify this indeed has been fixed.
I created this ticket based on #3302638-3: Telephone International Widget 2.0.0 stable release plan → . I assumed the front-end validation from 🐛 Doesn't work with multiple phone fields Needs work was sufficient, but I realize now that this is referring to back-end validation.
Absolutely! Thanks for calling that out, @johnpicozzi. I'll take a moment to look through the other tickets which may have influenced the total sum of changes captured here. Thanks everyone for your patches, MRs, and review.
This is the last holdout for a rc2
and
📌
Release 2.0.0
Needs review
.
The composer.libraries.json
file has been removed in
🐛
Doesn't work with multiple phone fields
Needs work
.
Captured in 🐛 Doesn't work with multiple phone fields Needs work . Thank you for this find!
As part of the work done in
🐛
Doesn't work with multiple phone fields
Needs work
, we load the utils.js
as part of the *.libraries.yml
.
The composer.libraries.json
has been removed as part of
🐛
Doesn't work with multiple phone fields
Needs work
. See that ticket for details.
Fixed with 🐛 Doesn't work with multiple phone fields Needs work .
@kerasai, I was still seeing this issue during my tests of 🐛 Doesn't work with multiple phone fields Needs work .
Got a test added and refactored the work from the above patches and MR !8. Good job and thanks , everyone.
Please, someone review MR !9.
Never mind. That intlTelInputWithUtils.js
doesn't even come with the package. Must be outdated docs.
Option 1: intlTelInputWithUtils
If you're not concerned about filesize (e.g. you're lazy loading this script), the easiest thing to do is to just use the full bundle (/build/js/intlTelInputWithUtils.js), which comes with the utils script included. This script can be used exactly like the main intlTelInput.js - so it can either be loaded directly onto the page (which defines window.intlTelInput like usual), or it can be imported like so: import intlTelInput from "intl-tel-input/intlTelInputWithUtils".
-- https://github.com/jackocnr/intl-tel-input?tab=readme-ov-file#loading-th...
Given this is loaded when the widget is used, let's adopt the above implementation in lieu of the changes made from #11.
Merged 📌 Remove the composer libraries workaround Active . This ticket can be updated to reflect any desired changes.