Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Follow-up to #2880149: Convert taxonomy terms to be revisionable
Problem/Motivation
In #2880149: Convert taxonomy terms to be revisionable taxonomy terms got a publishing status field, but no UI is yet available for users to publish / unpublish taxonomy terms.
Things that should be taken into consideration:
- #2892304: Introduce footer region to ContentEntityForm
- Whats being done on #2834546: UI for publishing/unpublishing block_content blocks
Proposed resolution
Implement the same solution as for nodes, content blocks etc.
- Add a published checkbox on the taxonomy term edit page
- Add publish and unpublish system actions in preparation to using views, and so that users can build their own UIs
published column on vocabulary overview pageSee #35, #36bulk operation actions for publishing / unpublishing termsSee #35, #36
Remaining tasks
User interface changes
Adds a published checkbox to the taxonomy term edit page.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#117 | Screenshot from 2019-06-21 17-06-01.png | 33.96 KB | Manuel Garcia |
#110 | 2899923-110.patch | 16 KB | Manuel Garcia |
#109 | 2899923-109.patch | 16 KB | Manuel Garcia |
#109 | interdiff-2899923-107-109.txt | 811 bytes | Manuel Garcia |
#107 | 2899923-107.patch | 15.21 KB | Manuel Garcia |
Comments
Comment #2
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedFirs step, adding the published checkbox to the taxonomy term form.
Comment #3
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedA screenshot of the form.
Comment #5
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedForgot to roll it against 8.5.x...
Comment #6
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedUnfortunately the taxonomy overview page is not a view yet, so we cant use these on that page.
Comment #9
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedThis patch will have to make use of the new available footer region #2892304: Introduce footer region to ContentEntityForm
Comment #10
vijaycs85Here is an update
Comment #11
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedExcellent thanks @vijaycs85 for working on this!
Patch looks good to me, only this nitpick I can see
Unnecessary extra lines
Comment #12
vijaycs85Thanks for the quick review.
Comment #13
vijaycs85Comment #16
vijaycs85Test failing with:
The Taxonomy term entity type needs to be updated.
at
Drupal\FunctionalTests\Update\UpdatePathTestBase::runUpdates
does it mean we need to wait for #2880149: Convert taxonomy terms to be revisionable to get in?Comment #17
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commented@vijaycs85 - most probably - I would try applying both patches, generating a combined patch and uploading it here to see what the bot comes up with, if you want to see =)
Comment #19
chr.fritschRemove this classes and use the generic entity action plugins.
Remove this class and use the generic BulkForm. We did that for media and block_content as well. It's overkill to have this.
Comment #20
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedExcellent points @chr.fritsch - and now that #2880149: Convert taxonomy terms to be revisionable is RTBC, we should be in a good position to resume work on here.
First, just a reroll of #13 (3 way merge did the trick).
Comment #21
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedAddressing #19 and getting this in sync again with the latest patch on #2880149: Convert taxonomy terms to be revisionable (#139).
Here is what I did:
Comment #23
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedRe-adding the schema files for the actions, because of tests failing, but I dont think we should need them. Shouldn’t the entity schema provide this for us? (
action.configuration.entity:*:*
)Either way, something is missing here because the actions are not being registered for some reason.
Printing:
Gets me this:
And thus you cant even select the "Bulk update" views field when creating a taxonomy term view.
Comment #25
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedOK so that wasn't it. Removing the schemas that I re-added on #23, and adding the missing trait to Term.
Comment #28
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedHad to reroll the other issue's patch, so updating the combined patch here as well.
Turns out the patch we had here is way outdated, and things are much easier now in core for enabling published entities since the hard work is done on
EditorialContentEntityBase
, so this patch should be smaller now.I'm starting to consider whether we should just focus here on adding the published checkbox to the Term form, and use a follow up to add support for published/unpublished actions alongside the generic entity actions. Thoughts?
Comment #30
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedComment #31
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedLet's wait for #2981887: Add a publishing status to taxonomy terms to get in :)
Comment #33
chr.fritschBlocker is in :)
Fixed some tests.
Comment #34
jibranWe need an upgrade path for these changes.
Comment #35
chr.fritschThought a bit about #28:
I agree that we should only focus on the status checkbox in this issue, because having bulk operations for terms is much more work.
I guess we first have to replace the EntityListBuilder with a real view on the term overview page. The related issue for that might be #113344: Taxonomy term manager view
Comment #36
jibranUnfortuntely this is not possible until we add https://www.drupal.org/project/draggableviews to core.
Comment #37
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedI agree with #34, tagging appropriately, and setting to needs work for that.
Thanks @chr.fritsch for the reroll and tests fixes, looks good to me.
Re #35 - I agree too, though perhaps we can still add the actions here? That way people can still implement it on their own custom views?
Comment #38
chr.fritschRe #34: I don't think we need an upgrade path for this. After a cache clear the status field is already on all the term forms. So I added an empty post_update function to ensure the cache is cleared.
Comment #39
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commented@chr.fritsch I think what #34 means is that we need to update the form display configuration for this.
Something like what we're doing on #2834546: UI for publishing/unpublishing block_content blocks
block_content_post_update_configure_status_field_widget()
would work I believe.Here is the upgrade path & test for it, also updated IS with an updated proposed resolution and remaining tasks.
Comment #40
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedOf course it helps if you attach things.
Comment #41
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedComment #42
jibranDo we need to set up this as well in upgrade path?
Comment #43
chr.fritschI still think we don't need the update path. Running the
testStatusCheckbox()
with an emptytaxonomy_post_update_configure_status_field_widget()
function also passes.Comment #44
chr.fritschWe discussed this in slack with amateescu
That means #38 is the patch for further reviewing.
Comment #45
jibranThis settings change has nothing to do with installed base field definition. Yes, we are setting an option on BFD object but we are making changes to default form mode. Entity form modes are config entities and
setDisplayOptions
is making a change to the values of the config entity which should be replicated in the active storage so we need an update path for this.Comment #46
jibranWe should also bail out if status field is already visible we don't want to change the settings of already visible field.
Comment #47
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI don't see where we are making changes to the form mode config entity:
On the contrary, not touching the default form mode config entity in this patch allows their possibly custom values to remain the same (as configured by the site builder) so the fact that we don't override user configuration is a good thing.
Comment #48
jibranI think if the status field is already visible we shouldn't change the configuration of the status field in default form mode and if the status field is not visible then we should set up the default display option because after applying this patch and enabling the taxonomy module the user will get the status field visible in the default form mode so adding the upgrade path will allow us to keep the default behavior.
Let's test your theory
with the upgrade path test. If the test would fail then that would mean we needed to update the status field config in the form modes.Comment #49
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedOK then, that settles it... the patch to continue from (review/rtbc) is @chr.fritsch's one on #38.
Re-uploading here for clarity.
Comment #50
tstoecklerI do think we need to resave all the form displays, though, so that the
status
field will get added to the list of disabled fields at least. Otherwise people will have unexpected diffs in their config the next time they happen to save a taxonomy form display.Comment #51
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedThanks @tstoeckler for chipping in, so your vote is that we do need the update path added on #40?
How can we get consensus on this? We have patches with and without update path & update path tests...
Comment #52
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThinking a bit more about this, I think I agree with @tstoeckler that we probably should include the update path, but only add the
status
component to the form display if it's not already there (e.g. already configured by the site builder).Comment #53
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedOK, here is the patch with the update path & test, plus the check suggested on #52
Comment #54
jibranThe logic is quite convoluted but correct and I don't see how can we simplify this. Anyway, this is the last outstanding feedback so RTBC.
Comment #55
alexpottI would split this into two different if statements. That way it is more readable and more self documenting. Ie. don't update if it not for taxonomy terms. And then don't update if there is configuration for the status field already. Or perhaps this is better written the other way around. Ie.
We also need testing that the entity displays with existing options are not overwritten.
Since this is just minor logic re-work feel free to re-rtbc on a new patch.
Comment #56
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedHere is the logic rework mentioned on #55.
Comment #58
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedOops
Comment #60
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedmeh, not my day...
Comment #61
alexpottI think this should be &&
Plus as mentioned in #55 we also need testing that the entity displays with existing options are not overwritten.
Comment #62
alexpottPlus the code comment added in #55 is helpful as it explains why the condition.
Comment #63
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedThanks @alexpott - For now addressing the logic mistake and adding the helpful comment.
Still to do adding test coverage for entity displays with existing options not being overwritten.
Comment #64
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedComment #65
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedHere is the interdiff for the last patch. Setting to Needs review for the tests to run, this is still Needs work for adding coverage for entity displays with existing options not being overwritten.
Comment #66
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedOK here is a go at the missing test coverage.
Comment #68
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedThis should come back green now :)
Comment #69
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThe new test coverage looks good to me!
Comment #70
larowlanunrelated?
Comment #71
longwaveAll those changes were needed to fix the test fails discovered back in #28 on the "combined" patch and then fixed in #33. System module is needed for the action config entity schema, and this patch adds actions so the action counts have gone up!
Comment #72
larowlanthanks
Comment #73
AnybodyConfirming RTBC. Thank you very much! I added a test runner against 8.6.x to see if this patch an be used in that branch already for stable users who need that functionality (backported). Can this be part of a minor 8.6.x release or will it definitely be 8.7.x?
Thank you all very much!
Update: 8.6.x passed!
Comment #74
tstoeckler@Anybody: As this is a feature request and not a bug fix, it will not be added to 8.6.x.
Comment #75
AnybodyWe're using this in production on 8.6.x now - successfully. Thank you all very very much!
Comment #76
AnybodyI just ran into the issue that the published checkbox state is not saved (enabled to disabled) when using content translation.
I managed to uncheck it by a workaround (but that is only possible for one language, so no solution): Also uncheck "This translation is published" AND uncheck "Published", then save... then the "Published" checkbox is saved unchecked.
Otherwise the taxonomy term stays published and the checkbox is checked again after save (reload).
I had that situation on our live environment together with other modules so could someone else please test that or write a test?
Comment #77
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedThanks @Anybody for reporting on that!
In this patch we're just mimicking what we did for the node form, which makes me wonder... does this happen with nodes as well?
If the answer is no, I suppose we should look into why not, so as to identify the solution here. I had a quick look and I couldn't see anything obvious, but then again I'm not very familiar with content_translation module...
Comment #78
BerdirSee #2971390: Make exposure of translation meta fields conditional, those ui elements are currently hardcoded, the patch there tries to make them more dynamic. Right now, it's just the node translation handler that overwrites some of that logic and even that is partially wrong.
Comment #79
Anybody@Manual Garcia: I just tried that in the same isntallation and unpublishing / publishing a node works as expected. So this problem doesn't exist for nodes. Thank you @Berdir so that's the cause for these problems?
I can confirm this problem still persists and think THIS issue can not be completet until that problem is finally solved.
Comment #80
andypostComment #81
jedgar1mx CreditAttribution: jedgar1mx commentedI did notice that even though the taxonomy is unpublished, the taxonomy term page still accessible. Node have an item in permission View own unpublished content. Shouldn't taxonomy has the same ability to limit access to unpublished terms? Thanks
Comment #82
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedJust a reroll of #68 for now.
Comment #83
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedOK I am mimicking here what node does on NodeTranslationHandler.
Tested it locally with a couple of languages, seems to work fine, let me know what you think :)
Comment #84
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commented@jedgar1mx re #81 yes - there is work going on to do this in a more generic way, see #2809177: Introduce entity permission providers
Comment #86
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedAdding coverage for the last change mimicking again what node does.
Comment #87
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedComment #88
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedSorry for the double interdiff, good one is https://www.drupal.org/files/issues/2018-12-16/interdiff-2899923-83-86_0...
Comment #89
Anybody@Manuel Garcia, thank you very much! If I read the patch correctly, the problem from #76 should be solved with this?
If yes, I'd retest the patch manually. Any other known missing pieces in the patch or tests?
Comment #90
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commented@Anybody yes, I think it should fix #76, please do let us know if it does/doesn't :)
If the problem detailed in #76 is now fixed, then the next step here now would be a code review from the changes introduced since #68.
Comment #91
AnybodyThank you very very much, the patch from #86 works perfectly and solves the problem described in #76! GREAT NEWS!
RTBC +1 from manual testing! No other related issues found!
Comment #92
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commented@Anybody thanks for testing!
This was set to RTBC before @Anybody found a bug, which is now fixed as he has manually verified. I'm tempted to set this back to RTBC for commiter review, however after reading what's going on on #2971390: Make exposure of translation meta fields conditional I'm also tempted on postponing this issue on that one, so that we don’t replicate the pattern of node module when it comes to the translation published status... ¯\_(ツ)_/¯
Comment #93
AnybodyPerhaps RTBC + a note / reference to the issue in the RTBC Comment and in the issue description?
Comment #94
AnybodyRTBC as of #92. Now waiting for #2971390: Make exposure of translation meta fields conditional
Comment #95
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedReroll of #86 (which included an unrelated file).
Comment #96
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedComment #98
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedFixing tests...
Comment #99
jedgar1mx CreditAttribution: jedgar1mx commentedUpgrade to 8.6.8 broke with old patch. But #98 seems to have fix it, thanks Manuel.
Comment #100
AnybodyI can confirm #98 works with latest Drupal core. Re-Confirming RTBC.
Comment #102
BerdirThis needs a reroll but the 8.6 patch was somehow preventing it from being set back.
Comment #103
andypostRe-roll for conflict in use
Comment #104
jibranThere are some @todo about this issue in core now after #2880149-291: Convert taxonomy terms to be revisionable.
Comment #105
andypostPartially restored test and changes from #2880149-291 except validation which needs work as a test fails
Comment #107
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedI think the content moderation integration should be done as a follow up, to keep this patch simpler as it only deals with the taxonomy ui itself.
Re-uploading #103 here for clarity.
Comment #108
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedCreated the followup issue for re-enabling CM integration for terms: #3047110: Enable Content Moderation integration for taxonomy terms
I think #107 is good to go, again!
Comment #109
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedThanks @amateescu - updating the
@todo
here to point to the follow up.Comment #110
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedJust a simple reroll, 3way merge did the trick.
Comment #111
ivnish CreditAttribution: ivnish commentedPatch works fine! I want it in the Drupal core :)
Comment #112
merauluka CreditAttribution: merauluka at Mediacurrent commentedAlso working for me against 8.7.3. Nice work everyone!
Comment #114
Gábor HojtsyThanks all for fixing the concerns raised!
Committed 8e31070 and pushed to 8.8.x. Thanks!
Comment #115
Gábor HojtsyComment #116
heddnI'm getting failures on contrib testing: https://www.drupal.org/pift-ci-job/1327224. I thought it was a head failure and I could ignore, but happened again last night.
Comment #117
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedre #116:
On https://www.drupal.org/pift-ci-job/1327224 I can see this error:
But as far as I know we don't need to add a schema for these since they're using the generic entity actions? See #2916740: Add generic entity actions for context.
I went and checked with
config_inspector
on a fresh install of 8.8.x and they show as correct, so I don't know what's causing that error.Comment #118
heddnStill getting test failures: https://www.drupal.org/pift-ci-job/1333904
Comment #119
vijaycs85@heddn fix is available at #3065517: Fix tests on 8.x-4.x HEAD
Comment #120
jedgar1mx CreditAttribution: jedgar1mx commented#110 works thanks.
Comment #122
alexandersluiter CreditAttribution: alexandersluiter commentedWill this be committed to 8.8?
Comment #123
Gábor HojtsyIt was already committed in June, see #113.
Comment #124
cilefen CreditAttribution: cilefen as a volunteer commentedI think #3083609: The new "Published" checkbox on the forum edit page doesn’t do anything may be related to this issue. Is it?
Comment #125
Ghost of Drupal PastConfirmed working for 8.7.
Comment #126
jedgar1mx CreditAttribution: jedgar1mx commentedRemember to remove patch before upgrading to 8.8
Comment #127
effortDee CreditAttribution: effortDee commentedI have imported thousands of terms from one site in to a new Drupal 8 site and all of them are unpublished and I didn't realise.
Anyway, not to worry, but is there a way to bulk publish taxonomy terms or do I have to go through them all individually?
Comment #128
BerdirI assume you did that programatically? You could either try to update it directly in the database tables and clear cache (and create a backup first), or load all terms, set to published and save again, that could take a while though.
Comment #129
AnybodyOr create a temporary taxonomy term view with a https://www.drupal.org/project/views_bulk_operations field to set entity values / published status, if you want to solve this in UI. I guess that should also work.
Comment #130
effortDee CreditAttribution: effortDee commentedEnded up figuring it out straight after this with VBO yes and "bulk change" field in a view.
Got it all done in a few minutes, thanks Berdir and Anybody!
Comment #131
Kristen PolIf anyone needs a patch for 8.7.13, please see #3127752: UI for publishing/unpublishing taxonomy terms (version 8.7.13 patch).