^^
Part of meta-issues
#1788918: [GTD] [META] Prepare for Twig main engine core inclusion until December 1
#1802750: [Meta] Convert configurable data to ConfigEntity system
Background Information
Config entities should use enforceIsNew or different way to ensure that they are not saved, details #1588422-139: Convert contact categories to configuration system
Related issues
#1480854: Convert operation links to '#type' => 'operations'
#1751244: Convert taxonomy module widgets to Plugin system
#1891690: Use EntityListController for vocabularies
#2005778: Convert language_admin_overview_form to a Controller
Comment | File | Size | Author |
---|---|---|---|
#170 | 1552396-interdiff-170.txt | 2.3 KB | andypost |
#170 | 1552396-core-voc-170.patch | 159.86 KB | andypost |
#166 | 1552396-interdiff-166.txt | 1.5 KB | andypost |
#166 | 1552396-core-voc-166.patch | 157.77 KB | andypost |
#164 | 1552396-interdiff-164.txt | 3.2 KB | andypost |
Comments
Comment #1
sunOh my. This his a huge undertaking. Spent the past 6 hours exclusively with this...
Comment #2
chx CreditAttribution: chx commentedAmazing work!
Comment #3
aturetta CreditAttribution: aturetta commentedVery clean, I think this simplifies a lot of work on taxonomy terms.
Maybe wrong, but I think the patch includes a couple of changes to image.module that are not related to this issue.
Comment #4
Berdir"General error: 3 Error writing file './drupaltestbotmysql/simpletest555283search_dataset.frm' (Errcode: 28)"
Looks like the testbot file system is full.
Comment #5
joachim CreditAttribution: joachim commentedIs there more context on this task anywhere? We converted vocabs from serial IDs to machine names for D7 so I'm a bit confused as to why they're going back the other way now.
Comment #6
jthorson CreditAttribution: jthorson commentedFails on testbot ... here's the first entries in the error log ...
Comment #7
sun#1: drupal8.taxonomy-config.1.patch queued for re-testing.
Comment #8
sunRe-rolled, rebased, merged, and conflict-resolved against HEAD, which was a super-major PITA due to #1533022: Convert taxonomy.module entity classes to PSR-0 changing type hints, phpDoc, and whatnot for vocabularies all over the place. :(
This will most likely still blow up the testbot, but I really wanted to get this re-rolled. Sorry @jthorson :-/
Comment #9
andypostshould be == not =
Do you really wanna remove cache?
Regression? why there's no drupal_alter()?
This api calls loader function? also why there's no room to alter?
comment wtf? trailing whitespace
Comment #10
andypost#8: config.taxonomy.8.patch queued for re-testing.
Comment #12
sunSpent the last couple of hours on this patch, converting tons and tons of calling code and tests, only to realize in the end that the actual problem is that $vocabulary is sometimes an array (the config data) and sometimes an object (DrupalConfig $config object).
The original intention was to convert everything to just simply the config data array. This keeps the configuration system mostly out of the module layer implementing the surrounding business logic.
However, this inherently means that the config data cannot contain any meta data that may be needed for business logic in module implementations.
Example:
Switching everything from arrays to $config objects... will be a monster undertaking (even speaking of this single patch only), since that inherently means this:
Now.
How about a crazy idea?
What if we just simply kept the entity classes for these configuration objects?
I.e., Drupal\taxonomy\Vocabulary would remain as-is. Only the entity storage controller goes away. Because we literally swap out the storage, storage engine, and storage controller for vocabularies under the hood only.
I'm very very sleepy right now, but somehow, this makes a lot of sense.
Also, this seems to be fully in line with what we envisioned and discussed for #1448330-19: [META] Discuss internationalization of configuration
Comment #13
fagoSo you mean to implement a config entity storage backend? At least to me, that would be natural approach...
The entity api already offers the storage-related API that a high-level configuration management system should offer for object-like configuration items, so using it makes a lot of sense to me. It's also a DX++ as it means the already known interface (crud functions, hooks, ..) remains valid for configuration objects too.
Comment #14
andypostFor taxonomy vocabularies as a kind of semi-entity this approach looks natural but in case of contact forms #1588422-13: Convert contact categories to configuration system this looks like over-engineering but we still need to figure out a way to convert core parts by keeping in mind a internationalization. So I leave #1588422 open/active to care mostly about i18n
Comment #15
sun@fago: No, I don't want to add a dependency on the entity system. That would be wrong. It would make sense to depend on a generalized property system (then being used by fields, entities, and config), but alas, that doesn't exist yet, so at this point I'm rather thinking of forking the Entity base class and resembling it to act as Config base class.
Comment #16
sunAlright, attached patch retains Drupal\taxonomy\Vocabulary, but refactors it to extend a new ConfigObjectBase+Interface.
I think this makes. a. lot. of. sense. :)
Comment #17
sunAh-ha! standard.install is guilty for the testbot failures. That was pretty much my theory since the beginning - the testbot blows up when a test fails right within the setUp() method, before the test is actually executed.
(This also means that all the fatal errors in the review log of #16 point to test cases which are still using the Standard profile and should be converted to the Testing profile ASAP; see #1595028: Convert tests using Standard profile to use Testing profile instead)
Comment #19
sunAttached patch should fix most of the remaining test failures.
Comment #21
sun#1054162: Taxonomy bundles not supported by EntityFieldQuery (followup) introduced a entity query alter hook implementation that needs to be removed here.
Comment #22
Dave ReidSide note that the removal of hook_image_styles_alter() might need to be discussed or mentioned for a D7 to D8 update documentation on how you can accomplish the same functionality of that hook in D8.
Comment #23
sunre: #22: The removal of drupal_alter() for configuration object loads will not be discussed in this issue. It already was in the original #1270608: File-based configuration API. Configuration objects have to be idempotent, which means that config->load()->save()->load() yields the identical result. If you want to change configuration, save it. There will be no other way to do it. (Period.) It looks like we missed to remove this particular drupal_alter() in the initial/original config patch, despite the discussions we had. In any case, if you want to elaborate on this further, please create a separate issue, because the topic is totally irrelevant to this patch.
Comment #24
Dave ReidFair. I'm simply noting that even though that issue discussed it, this issue is actually removing it and once fixed should be noted in the module upgrade guide how it can be replaced.
Comment #25
catchHow doable is it to split the new API functions and/or the image style clean-up into a separate issue?
Comment #26
fagoInteresting. +1 for going the same API for doing the same things, still I'm not so sure about the necessity of the split. A common CRUD-Interface holding the common code/interface together might be an interesting way to explore.
Given that, it's likely we produce two separate code paths in all components supporting both, like form generation code.
@dependency: You mean, generally for config objects? I don't see why it should be just "wrong" and would appreciate if someone could explain the reasoning instead of just repeating it again and again :D
I mean, conceptually it totally makes sense, vocabularies clearly are "entities".
Comment #27
sun@fago:
First, configuration must be idempotent; i.e., config->load()->save()->load() must yield the same result.
Second, configuration is "namespaced". Which means that you generally have no business in my configuration object. You create your own, in the same way you used your own database tables or variables before.
These are the main reasons for why I did not move the module system related logic into the Vocabulary class, but instead, kept that in the procedural taxonomy_vocabulary_*() functions.
Admittedly, this is debatable to some extent. Because it inherently means that vocabulary configuration can be changed, sometimes notifying other modules, and sometimes not - depending on whether you loaded/saved the config object via Vocabulary or just plain config().
The general baseline of thought, however, is that all configuration should work regardless of bootstrap level and state. In fact, the configuration system aims to work as the very first bootstrap phase, DRUPAL_BOOTSTRAP_CONFIGURATION, in which pretty much nothing aside from settings.php exists.
In turn, that's why I'm very hesitant to introduce a dependency on the entire entity system (and in turn, all of its own dependencies).
But yes, that's totally the essence we need to discuss here. :)
Comment #28
fagoThanks!! Here are my thoughts:
True. I think it doesn't apply to all entities either, but i don't see a big problem in that. If you use it, the system should just go and try to create something new then - whatever that means for your storage backend.
Sure, entities should be too, i.e. you should get back what you store. So whether we need the load hook, is just a question of whether we allow modularly built objects - where we are at:
Hm, so you say it's not valid for any module to extend an existing configuration object? Right now, that's quite common: Think of all the serialized settings and options array, which contrib extensions use to easily add their settings. E.g. as it is for fields. How would that work then?
Imo, this would result in a rather bad DX. Moreover, it might result in lots of configuration changes being "lost" in the sense that modules miss them because the update happened via the "wrong channel". I do think that every configuration changes *has to* result in the associated hooks to be invoked, or the whole configuration API is becoming unreliable (and so a mess..).
That said, yes - configuration changes and reloading from disk need a high bootstrap level and I don't see a problem with possibly invoking entity hooks here.
Of course this has to be valid for the configuration system per se. But that doesn't mean we can introduce a system for handling configuration objects that runs on a higher level + stores to the config system.
There would be no dependency of the configuration system on the entity system. Modules, that would use the entity system to handle their configuration objects would have to depend on it, yes - but I don't see a problem with that.
Yes, I agree that the config of module setting forms doesn't match the entity concept, thus I don't argue for them to become entities.
Yep. I think, first it's important to lay out the needed and wanted "features" (hooks to be called, ..) for the configuration objects, then we can evaluate and decide whether the entity system would be a good fit or not.
Comment #29
xjmTagging for now. I do think we should spin off ConfigObjectBase into its own issue, though, because it's way out of scope here and merits consolidating with the work we did at tcdrupal for VDC. We can postpone this on that as needed.
Comment #30
sunAt this point, splitting ConfigObjectBase into a separate issue does not make sense to me, since that would inherently ignore the actual use-case.
That said, there's a major clash in terminology/arch-design. @beejeebus is "also" working on a ConfigObject in parallel, but that is going to be used internal within the config system only (i.e., contrary to this here, which is actively instantiated from the module implementation layer and used within, but not actually passed back to the config system).
Comment #31
sunNote that #1609760: hook_image_style_*() is not invoked for image styles upon Image module installation partially duplicates this discussion to some extent now. Might be worth to discuss the general architectural problem space over there first - but perhaps not. Let's try. I'll try to make sure that we don't duplicate, or otherwise mark that issue as duplicate.
Comment #32
xjmThis doesn't make sense to me. There's way more than one actual use-case?
My point is that the underlying "big idea" in this patch has lots of implications for CMI, the entity system, VDC, etc. and so there should really have a dedicated issue rather than several people working on similar solutions in parallel.
Comment #33
gddI completely agree with xjm, at the very least we should change the title of this issue to match the discussion that is going on.
Ultimately I don't really follow what happened in #12 that led to the idea that moving to typed config objects and a more entity-like api was a necessary change. I would really love some elaboration on that. In other issues I saw it discussed that this would allow the ability to centralize business logic in class-specific ->save() operations and the like, however in #1609760: hook_image_style_*() is not invoked for image styles upon Image module installation it appears there is some consensus forming that this would be unnecessary anyways. However if we had an issue that was just 'Implement typed config classes' we could discuss all these merits and demerits in one place rather than around 15 implementation issues you know?
Comment #34
sunPostponing on #1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc)
Comment #35
sunHere we go again.
Attached patch converts vocabularies from Entity to ConfigurableBase. Furthermore, it replaces $vid with the vocabulary machine name and drops $machine_name (like all patches before).
Code lives in the config-taxonomy-entity-1552396-sun branch of CMI.
Had to revert the EntityInterface/StorableInterface change for ConfigurableInterface, because Taxonomy vocabularies are using the EntityFormController already. We shouldn't have done that in the first place. If we really want/need a separation, then we need to introduce a truly new ContentInterface that extends EntityInterface. But as already mentioned in Munich, there's not going to be a need for that separation anyway.
Comment #36
tim.plunkettRather than do that, switch EntityFormController to use StorableInterface.
If you want a follow-up to change this back, please open another issue. We've already converted Views to StorableInterface, and burying this change in an unrelated issue is not fair.
Comment #37
tim.plunkettSeems that #1761040: Rename Storable, Entity, and Configurable to Entity, ContentEntity, and ConfigEntity would be a place to do that.
Comment #38
sunCreated #1761048: Revert StorableInterface/EntityInterface separation
Fixed stale machine_name in Forum module.
Fixed missing dependency on Config module.
Comment #40
sunFixed Standard install profile.
Comment #42
andypost#40: config.taxonomy.40.patch queued for re-testing.
Comment #44
andypostRe-roll, with a few fixes (mostly about class names)
EDIT: filed #1789722: Fix ConfigStorageController::save
Comment #46
andypostFixed
- some tests
- Vocabulary reorder (still Warning at taxonomy.module 1065)
uasort() [function.uasort]: Array was modified by the user comparison function - taxonomy_vocabulary_load_multiple()
- ConfigStorageController::save
Comment #47
Lars Toomre CreditAttribution: Lars Toomre commentedA small nit for if this gets re-rolled...
On the first couple of read throughs of the patch, this section was confusing. Perhaps enforceIsNew() is misnamed. However, it was confusing why in the isNew() section this was being called with a FALSE parameter. A comment above that line would be really helpful.
Comment #49
tim.plunkettThat entire hunk has no business being in this patch, it changes the behavior for all ConfigEntity implementations.
If there is a bug with it, there should be a separate issue with a test.
Comment #50
andypostI already filed #1789722: Fix ConfigStorageController::save
But this patch require this hunk while this broken
Changes:
- Fixed some tests
- There's no implementation of EntityStorageControllerInterface::loadByProperties() so a few lines are commented in test
- Sort of vocabularies throws Warning
- testTaxonomyVocabularyLoadReturnFalse() is commented out - make no sense
Comment #52
andypostCurrently we have no hooks for moment of config application so this issue should add a reaction for fields first to do something when some vocabulary object beed removed by applying new config
Comment #53
andypostThe VocabularyStorageController:postSave() called original:delete() before hook_update()
Comment #55
andypostFixed forum tests by disallowing changing machine-name for forum, I think we need to disable changing machine name because then we need massive update for
taxonomy_term_data
Comment #56
andypostLet's try disable vid change in form controller
Comment #58
andypostMost of tests should be green.
we need to call resetCache() to clean-up static
taxonomy_vocabulary_get_names
Commented out a test because changing of machine name is not allowed
Comment #60
andypostAnother round, most of tests are fixed
Config module is enabled in hook_update() to fix upgrade path tests, we still depends on #1785974: Move ConfigEntity into a Core component
Sorting still throws error:
uasort(): Array was modified by the user comparison function
so just prefixed with @ - as suggested in https://bugs.php.net/bug.php?id=50688Also needs D8MI review on default language for vocabularies
Comment #62
andypostSuppose tests should be green
Comment #63
tim.plunkettOverall this patch is AWESOME! Even with all of the update code we're removing 100 lines.
This could be rewritten as
$a_label = $a->label() ?: '';
, like we do for the new state() code.Beware, if #1785974: Move ConfigEntity into a Core component lands this will change back
So is this really what we want to do? Or would it make sense to globally renamed vid to machine_name or something that doesn't imply a serial ID?
It seems that half of this patch is renaming vid to machine_name, how much bigger/smaller would it be if we went the other way?
Comment #64
Gábor HojtsyI don't get "Re-visit later when property patch lands", since the config entities should have a language property as well already? The existing vocabularies are saved with the site default language as well right? I see at least that for forums in the patch.
Comment #65
andypost@Gabor, yes, because we have language upgrade tests. But I can't find a code which adds a langcode field for vocabulary... and I scare that my hack (assign a site default language) is wrong...
Here is a upgrade tests that we have
But here the vocabulary table does not have langcode field yet... which is strange...
EDIT
This change was useless
Comment #66
andypost@tim.plunkett:
1) good idea to replace all _sort helpers in core, with pointed pattern, but I think we need a separate issue for that.
2) yes I really wait for #1785974: Move ConfigEntity into a Core component
3) other way? the purpose to get rid of serial IDs... But we can change all vid to ->id() but I thinnk this just gives a performance regression
This debatable, I think we should disable change as #55
This is a debatable change, I think we need to call postSave() after module's logic
Comment #67
Crell CreditAttribution: Crell commentedNo, that's good API design. Using interface-defined methods instead of raw properties is what we should be doing, universally. Then what the internal property is named doesn't matter.
Unless you can show that there's more than a 10% performance difference along critical path code (which I highly doubt), don't bypass the method to access the property directly. This question illustrates one of the key reasons for that.
Comment #68
andypostLanguage upgrade for vocabularies was lost so reverted.
Ternary login changed to proposed for PHP 5.3
$a_label = $a->label() ?: '';
EDIT needs re-roll because of commited #1785974: Move ConfigEntity into a Core component
Comment #69
andypostJust re-roll with a few fixes depending on #1785974: Move ConfigEntity into a Core component
Also here's a 2 patches:
- noid - just re-roll
- withid - an attempt to change
Vocabulary->vid to Vocabulary->id()
where possible, and changeTerm->vid to use Term->bundle()
EDIT I think that everything that lives in taxonomy module should know a database storage, PK is first thing!!!
First 2 changes are filed as separate issues
#1796604: Make constructor parameters consistent
#1796618: Change style of ternary to PHP 5.3
Comment #70
Gábor HojtsyAs per the PHP bug report (https://bugs.php.net/bug.php?id=50688) this sounds like an error/exception down the line in the sorting function which might be related to your test data related to label(). That is really the only "action" that the sort function executes on the config entity.
Comment #71
BerdirHere's a quick review, haven't read all of the comments, just ignore it if I'm repeating something...
Is there a reason these weren't replaced with id()?
Typo, should be "available". Also not sure I understand this comment..
Commented out test method. Can this be removed or why is this?
More outcommented stuff, haven't read through the whole issue, what's the reason, do we want to commit it like this?
debug statements.
Just like the labels, I think this place should not use the id() method. It explicitly wants to edit the vid.
Is that comment/code still correct? vid is always set, isn't it?
Comment #72
andypostuasort() throws warning because no language_manager registered in drupal_container()
The first call to ::label() calls entity_get_info() which tries to find a language_manager... warning is generated in
ServiceCircularReferenceException is actually what described in https://bugs.php.net/bug.php?id=50688
About commented out tests:
testTaxonomyVocabularyLoadReturnFalse() now removed because vid is no more serial.
testTaxonomyVocabularyChangeMachineName is debatable because machine name change probably could be allowed...
Other asserts are commented out while there's no implementation for EntityStorageControllerInterface::loadByProperties()
About usage of ::vid and id() I think all vocabulary units should know it's fields/properties also using label() could give translated value of the label
Comment #73
tim.plunkettTagging.
Comment #74
xjmMeta issue for these: #1802750: [Meta] Convert configurable data to ConfigEntity system
Comment #75
andypostConfig entities should use enforceIsNew or different way to ensure that they are not saved, details #1588422-139: Convert contact categories to configuration system
Comment #76
sunI didn't have time to review the entirety of the latest patch since I last worked on it and don't have time right now.
So just this for now:
Please revert; always use id().
Comment #77
andypostSo changed back and merged head after #1480854: Convert operation links to '#type' => 'operations'
Comment #78
andypostAnd missed hunks with schema and dependency on config module
Comment #79
tim.plunkettThese look indented wrong, and why is it using ->name not ->label()?
Comment #80
andypost@tim.plunkett this change is right, actually it fixes indented lines :)
I think we should cleanup name<->label() in follow-up
Comment #82
andypostAdded changes from #1751244: Convert taxonomy module widgets to Plugin system
Comment #83
andypost38 files changed, 374 insertions(+), 502 deletions(-)
Comment #84
andypostFinally uncommented tests that were broken
Fixed vocabulary name change (cache invalidation and stupid usage of $update variable)
Comment #84.0
andypostUpdated issue summary.
Comment #84.1
podarokUpdated issue summary.
Comment #85
andypostRe-roll after #1739928: Generalize language configuration on content types to apply to terms and other entities
Comment #86
andypostChanged name => label() as #79
Patch is growing, and requires reviews
Comment #87
andypostRe-roll after #1739928: Generalize language configuration on content types to apply to terms and other entities
Comment #89
andypostViews in core now requires to fix it's plugins and probably tests.
Comment #90
tim.plunkettIt probably has integration for the DB table and columns.
Comment #91
andypostNot sure how to remove vocabulary table, but filter on vocabulary works for term view
Comment #93
andypostShould pass tests
Filed #1821274: Add back ability to sort on vocabulary weight and name
Dropped legacy staff and renamed vocabulary all over to vid
Comment #94
dawehnerJust a small review of the views changes. In general that's great work!
This doesn't look right, it should be vids
I really like this changes!
There should be some changes on the taxonomy.views.inc as well. I guess using vocabulary as a code flag sounds legal.
Good idea to add a @todo!
Comment #95
andypostFixed #94
Comment #96
andypostre-roll after #1820834: Change #type machine_name default 'source' element to 'label'
Comment #97
dawehnerUnless of these comments i couldn't find anything in this huge patch.
It is actually great that we don't have this dependency anymore.
I don't actually see this variable used, is this by intention?
Just in general it might make sense to move this to the upper class once.
Isn't the config module about the admin interface, so we don't need this in the standard profile?
Comment #98
xjm#96: 1552396-core-voc-96.patch queued for re-testing.
Comment #100
tim.plunkettRerolled for both the entity type plugins and EFQv2.
Comment #102
andypost@tim.plunkett please provide interdiff to follow your changes
Comment #103
tim.plunkettYou can't provide interdiffs when doing rerolls. There were no changes made. You can diff the patches if you like.
Comment #104
andypostOther changes #97:
- removed config.module from standard profile
- $new_vid removed as unused variable
I don't think that drupal_static_reset('entity_get_info'); should be moved in this patch
Comment #106
dagmar#104: 1552396-core-voc-104.patch queued for re-testing.
Comment #108
andypostNow it's green!
Comment #109
andypost#104: 1552396-core-voc-104.patch queued for re-testing.
Comment #111
andypostEntity translation test is broken somehow because translation_entity_entity_insert() trying to insert into translation entity but vid is not integer.
Patch introduces a bit of clean-ups and tries to fix the new test
PS: current diffstat
55 files changed, 485 insertions(+), 824 deletions(-)
Comment #112
swentel CreditAttribution: swentel commentedI've hit that Entity translation installation as well on the field api cmi conversion patch - see http://drupal.org/node/1735118#comment-6689412 - I had to change the entity_id to column because the field instance doesn't haven id anymore, but a uuid, this might be the case here as well.
Comment #113
andypost@swentel I think better replace entity_id with entity_uuid - related #1726734: Replace serial entity IDs with UUIDs in URLs, or even globally?
Comment #115
plach@andypost:
@swentel is right: the UI test failure is caused by a wrong schema definition, as we didn't take into account config entities. Moreover #1832000: Entity translation metadata should be stored only for translation enabled entities/bundles. Sorry for getting in the way :(
I think the same fix proposed for the Fields->CMI issue should be ok. The other test failures look unrelated to me: that test should rely only on the test entity, the test storage controller and efq. No idea of what is happening there.
Triggering a retest, as I could not reproduce the failures here since the drupal installation is failing.
Comment #116
plach#111: 1552396-core-voc-111.patch queued for re-testing.
Comment #118
BerdirI've confirmed that with my patch in #1832000: Entity translation metadata should be stored only for translation enabled entities/bundles the tests are green. Can we get a quick RTBC over there to unblock this?
Comment #119
Berdir#1832932: translation_entity_entity_insert() assumes entity IDs are integers was commited and should also unblock this. Let's try a re-test.
Comment #120
Berdir#111: 1552396-core-voc-111.patch queued for re-testing.
Comment #122
andypost#111: 1552396-core-voc-111.patch queued for re-testing.
Comment #123
YesCT CreditAttribution: YesCT commentedTest bot comes back green on #111 from the recent re-queueing.
Reading back a bunch of comments, this looks like it's been reviewed and might be RTBC. @andypost do you agree? or would you like a final look at #111? Maybe by @dawehner or someone already familiar with this...
Any feelings about what is needed to get this committed?
I thought updating the issue summary might help
and posting a version of the patch with just the tests and no fix to show it fails the test bot.
Comment #124
andypostChasing head. no changes
@YesCT I think patch RTBC but suppose still needs review from @sun, the whole thing should be commited and issue summary is enough - we are converting vocabularies as part of #1802750: [Meta] Convert configurable data to ConfigEntity system
Comment #126
andypostThe bots sometimes fail to write config files. So retest helps
Comment #127
andyceo CreditAttribution: andyceo commented#124: 1552396-core-voc-124.patch queued for re-testing.
Comment #128
andypostmerge head
Comment #129
amateescu CreditAttribution: amateescu commentedWow, this is an amazing patch. I could only find two small things:
The class name is actually 'Drupal\Core\Config\Entity\ConfigStorageController'.
$bundle should default to NULL.
Otherwise, this looks RTBC to me.
Comment #130
andypostThanx for review. Fixed
Also I found that merge caused to exclude removal of views handlers
Comment #131
amateescu CreditAttribution: amateescu commentedUgh, sorry, I forgot to say that first issue must be fixed throught that file, it wasn't only one occurence.
Comment #132
andypostThanx a lot
Comment #133
amateescu CreditAttribution: amateescu commentedFound a couple more places where we needed to use id() and bundle(). Since we have issues for the todo's introduce by this patch, I'll RTBC if the tests come back green.
Comment #134
sunThis looks excellent.
You've clearly taken this issue over and it very much looks like we're very close to completion here.
I only have a few relatively minor issues, which are (hopefully) easy to address:
Off-hand, I do not understand why this condition is limited to an existing 'forum' vocabulary — if Forum module is installed, then the configured vocabulary ID, regardless of whether it already exists or not, should get the appropriate treatment.
TBH, I have no idea why anyone would ever want to sort taxonomy terms by the weights of their associated vocabularies. The weight of vocabularies has a single and concrete purpose only; it controls the order of vocabularies if they appear in e.g. select form elements on administration pages and perhaps also node/add or node/%/edit.
Speaking of, is there are actually any remaining case in which vocabularies appear in that way...? ;)
In short, I'd recommend to remove this @todo and the sort option, without replacement, and close that issue as won't fix.
mmm, I do not understand these @todos and why the assertions have been commented out.
Ah. loadByProperties() isn't supported for config entities, since there is no EFQ support for config entities yet, I see.
Gotcha. Let's simply remove those assertions, please. If that support will be added, we will have dedicated tests for config entities. No need to duplicate those assertions here, and actually, it's guaranteed that no one will remember that we've commented out these lines here. Thus, /dev/null those lines.
Hmmm... It was possible to rename vocabulary machine names before. The entire infrastructure for that is actually in place alraedy, so I don't really see the reason for why we'd suddenly want to disallow that?
$vid will blow up as soon as there might be additional config objects that share the vocabulary ID, but have an additional suffix; e.g.,
taxonomy.vocabulary.ID.blah
I guess it's OK to go with this for now since we generally need to correct all code that attempts to extract the config entity ID from a config object name, but I wanted to make sure to mention that. The correct code involves some fancy explode() trickery on both the config_prefix and the $config_name, so yeah, let's just simply leave that for some other issue.
Ouch. Did we really allow 255 chars for a vocabulary machine name in D7...? That is going to bite us, badly. :-/
Anyway, that's edge-case stuff to be figured out later.
We should, however, create a follow-up issue to limit the maximum length of the new vocabulary IDs to 64 or so.
(and continuing) These changes look unnecessary and actually invalid to me — the argument is still a vocabulary ID ("vid") and we're still limiting results on that vid.
This actually sounds like a blocker to me — I still do not really understand the bug being reported in the referenced issue, and I also do not understand how there could be a vocabulary without a weight property (which in turn would trigger the PHP notice that is being claimed).
That said, I'll try to look into that issue once more to see whether I can make any more sense of it today. If you have any additional clarifications you can share over there, that would be helpful and appreciated.
Comment #135
Gábor HojtsyReviewed the patch from the point of view of the multilingual features. I was delighted to find language support intact which is key. Good stuff :)
Comment #136
andypost#134
1) forum_form_taxonomy_vocabulary_form_alter -fixed
2) vocabulary sorting - could be useful to group/order terms - as is
3) loadByProperties() - removed commented tests
4) machine name change - probably it work for now, so removed #disable
5) bundle changed back to $vid
6) taxonomy_vocabulary_sort() - added patch without suppressing error, let's see (actually the problem in language manager)
EDIT: Locally I got broken 1 test "taxonomy vocabulary interface" because of warning
Drupal\taxonomy\Tests\VocabularyTest->testTaxonomyAdminChangingWeights()
taxonomy_vocabulary_sort()
Comment #138
YesCT CreditAttribution: YesCT commented#136: 1552396-core-voc-136.patch queued for re-testing.
Comment #139
YesCT CreditAttribution: YesCT commentedthe second patch fails to apply, locally and by the test bot
http://qa.drupal.org/pifr/test/390183
looks like a lot of file locations changed.
Comment #140
YesCT CreditAttribution: YesCT commentedrerolling I had this conflict:
in core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TermTranslationUITest.php
I merged it as:
Comment #141
YesCT CreditAttribution: YesCT commentedComment #143
andypostI filed #1848904: Bundles cannot be specified in Entity Translation tests
Here's a fixed test that includes fix from issue above
Comment #144
YesCT CreditAttribution: YesCT commented@andypost this one passes the testbot. the patch in #1848904-6: Bundles cannot be specified in Entity Translation tests (patch 5) failed tests. Which version of the other patch does this patch include?
Comment #145
andypost@YesCT #143 patch is different from #1848904-13: Bundles cannot be specified in Entity Translation tests because after conversion term needs just a bundle
Comment #146
andypostPatch includes changes in #1848904-13: Bundles cannot be specified in Entity Translation tests
Comment #148
dagmar#146: 1552396-core-voc-146.patch queued for re-testing.
Comment #149
andypostRe-roll after #1848904: Bundles cannot be specified in Entity Translation tests
Comment #151
andypostFixed broken tests
Comment #152
aspilicious CreditAttribution: aspilicious commentedCan we mark this rtbc? I couldn't find any issues but I'm not sure all the things mentioned in #134 are fixed.
Comment #153
andypostThe only thing from #134 still there
And
I filed #1853304: Limit the maximum length of the new vocabulary IDs to 64
Comment #154
tim.plunkettI reviewed this, and it looks sound. taxonomy_vocabulary_sort() is the only ugly part of this, and there is a @todo with an issue (#1799600: Add test of sorting for configuration entities).
Let's get this in!
Comment #155
fagoI also took a look and everything looks great, beside the already noted sorting todo which already has a follow-up!
The only minor and obviously not commit-blocking point I found:
Should have a leading \ at the namespace (if changed already...). Others also.
Comment #156
andypostRe-roll after #1774332: Better handling of invalid/expired cache entries
PS: also fixed #155
Comment #157
catchI only found minor issues, but I think the dropping of the taxonomy_vocabulary from existing sites is a blocker - some contrib modules or custom code might still reference vid and it costs us nothing to keep the table around until 9.x.
This pre-dates the taxonomy term reference field. When preparing $node->taxonomy, taxonomy module would query all terms for the node, then order them by vocabulary weight first, then term weight, then name (and it was a horrible query too). I was surprised to see support for this in core any longer at all, but it looks like Views trying to support Drupal 6-style term rendering for field displays..
.
We're relying on the configuration storage never changing during the entirety of the Drupal 8 cycle by using config() here. That's not introduced in this patch by any means, but seeing it again I've opened #1856972: config() isn't safe to use during minor updates if there's a change to configuration storage.
This doesn't leave any historical information around for modules that were relying on vid. So can we really get away with dropping the table in the next update? I think it'd be better to leave it for existing sites then drop it in Drupal 9.
So, so happy to see that horrible function (which I added) gone.
This seems like it'll be needed a lot with prefixed config names - maybe a follow-up to add an additional helper?
It looks like sun's feedback here wasn't really answered. Do we actually need to order taxonomy vocabularies in the UI at all any more? fine with removing that in a follow-up.
bye bye :)
Comment #158
sun#156: 1552396-core-voc-156.patch queued for re-testing.
Comment #160
catchPre-emptively added the vocabulary table to #1860986: Drop left-over tables from 8.x.
Comment #161
sunIn trying to decipher @catch's review in #158 and briefly discussing with him in IRC, it looks like almost all of the review issues are off-topic for this particular issue and need to be handled in separate follow-up issues (which partially exist already).
The conclusion is:
1) We don't want drop the taxonomy_vocabulary table for D8. Just leave it intact. Let's simply remove that module update function.
2) One or two or three of @catch's review issues are asking for dedicated follow-up issues, which may not exist yet:
A) The Views sort handler on vocabulary weight should be dropped. Separate issue.
B)
config()
in module updates is covered by #1856972: config() isn't safe to use during minor updates if there's a change to configuration storage & Co.C) We should not change the taxonomy_vocabulary table (and replace vid with the machine name). Let's simply leave the table as-is and perform the conversion from machine_name to vid in the migration to config. This is the only part that needs to be done for this issue (next to not dropping that table).
D) Add an optional argument for
config_get_storage_names_with_prefix()
to return results without the passed-in $prefix. Was discussed in the past already, still a good idea. While being there, rename that ugly function toconfig_list_all()
, thanks. Separate issue. (which should be created, but which should also not block this issue.)E) Let's re-evaluate the need for weights on vocabularies in a follow-up issue. This issue should be created ahead of time/commit.
That's it. :)
Really, don't lose hope. We're this --><--- close to get this committed :) Thanks everyone for making this happen!
Comment #162
andypost@sun thanx for "decipher" but I don't a get a part about weight - if we leave taxonomy_vocabulary table orphaned so how the weight handler should operate? current patch just drops it to pass tests
Comment #163
Anonymous (not verified) CreditAttribution: Anonymous commentedI just noticed that the Vocabulary class documents $vid as an @integer, but strings like 'tags' are actually used. I'm not sure when that inconsistency was introduced, it may have been before this patch.
Comment #164
andypostHere's a merged head and fixes:
1) vid is string per #163
2) if we keep vocabularie's table so returned back schema definition and update hook
Comment #166
andypostFixed new efq-test
Comment #168
andypostBot was fixed, now passes tests
Comment #169
BerdirDon't re-add the schema definition of the table. We don't want to add it for new installations, just not delete it when upgrading. People might have custom tables with a vid reference that they need to upgrade somehow. Or altered the table, or still need the data in it for some other reason...
Comment #170
andypostDone
Comment #171
catchLooks good to me now.
Comment #172
catchAlright. Committed/pushed to 8.x, this will need a change notice.
Comment #173
Gábor HojtsyYay, this is a great change making vocabularies the first config entities to have a language selector and have language configuration that applies to entities. Next up content types? #111715: Convert node/content types into configuration
Comment #174
patrickd CreditAttribution: patrickd commentedThis patch seems to have broken MyISAM support
#1871032: Taxonomy module fails to install on MyISAM due to too long schema index
Comment #175
Gábor HojtsyComment #176
yched CreditAttribution: yched commentedKudos for the awesome work !
Just a note that the upgrade path creates taxonomy.vocabulary.*.yml entries, but not the corresponding manifest file & entries.
No biggie, all the "convert X to config entities" tasks that got in so far currently fail to do so too.
We'd need the helper function added in #1817182-12: Add upgrade path tests for contact category config conversion
Comment #177
webchickCan we please file a follow-up to create tests for this/throw some kind of exception when this happens? This is at least the second or third time we've missed this.
Comment #178
andypostHere is a initial change notice: http://drupal.org/node/1873210
Comment #179
agentrickardFixed up with @andypost and @DraveRobber.
Comment #180
tim.plunkettI read through it, looks good to me.
Comment #181
xjm@webchick, I filed that followup the last time. ;)
#1852896: Throw an exception if a schema defines a key that would be over 1000 bytes in MySQL
Comment #181.0
xjmUpdated issue summary.
Comment #182
andypostFiled follow-up #1891690: Use EntityListController for vocabularies
Comment #184
andypostFollow-up to convert path #1978112: Convert taxonomy admin path to follow other core entity patterns
Comment #184.0
andypostUpdated issue summary.
Comment #184.1
andypostUpdated issue summary.