[SDPAP-7925]add config files #34
Conversation
There was a problem hiding this comment.
@yeniatencio please look into the comments. Also please add some behat test for this field if there isn't any.
| @@ -1,16 +1,19 @@ | |||
| uuid: 53e1711e-9e06-497f-8b29-7361d0bd0273 | |||
There was a problem hiding this comment.
please don't add uuid for the config/install. This uuid represents the unique id for a specific site, so installing it for different project will create issues.
There was a problem hiding this comment.
Thanks @MdNadimHossain , my bad, I didn't realise it was added it. I have removed them. Thanks.
| - taxonomy.vocabulary.department | ||
| - taxonomy.vocabulary.organisation | ||
| _core: | ||
| default_config_hash: '-ExNyXJOfyV6z82S2g3GreY6Vnqw4F48Y1fJ5BVTRWw' |
There was a problem hiding this comment.
this one as well, remove it. That gets generated when it exported for a specific site.
| @@ -1,9 +1,16 @@ | |||
| uuid: d0021eb1-fa3f-4711-a1d0-2a2399d6c0b5 | |||
| third_party_settings: | ||
| field_permissions: | ||
| permission_type: public | ||
| _core: |
src/TidePublicationOperation.php
Outdated
| * @param string $vocabulary | ||
| * The new vocabulary. | ||
| */ | ||
| public function updateFieldPublicationAuthorsVocabulary(Config $config, string $vocabulary) { |
There was a problem hiding this comment.
why are we separating this like a helper function ? This needs to be used only once, after that this code will just sit here. Better to put it in update hook, cause after that there is no use of it. Also the values you set are constant, that also means this function can not be used for any other fields.
There was a problem hiding this comment.
Hey @MdNadimHossain , I am following Matthew's suggestions from this pr
@krakerag, @MdNadimHossain let me know guys how should I proceed then.
There was a problem hiding this comment.
I don't think this applies here, cause this function not going to be used after updating this field. Also for fresh install in the new CMS the update the updates will get in from the config/install for these fields. If there are scope where you can reuse this function then it make sense to separate it in a class. But this one not needed in the install hook and no other use case of reusing it as well.
CC: @krakerag
There was a problem hiding this comment.
This is an interesting one. All update hook code is throwaway code, but we established a pattern in tide_search and tide_core to minimize the code in the .install file, for the preference of putting code in a Helper or Operation class that lives in the src folder.
I think @yeniatencio is just following that established pattern here.
That said, I am happy to see either - I don't feel like being strict on install file rules is going to gain anything from a code readability standpoint.
I would be more strict on the createVocabulary method being in a reusable code location which is good on L86 below.
| * @param string $vocabulary | ||
| * The new vocabulary. | ||
| */ | ||
| public function createVocabulary(string $vocabulary) { |
There was a problem hiding this comment.
This helper function we can keep, cause this can be used for creating any other vocabulary.
There was a problem hiding this comment.
This is a good example of a code reuse pattern, good one @yeniatencio
Hey @MdNadimHossain , Publication author field is not a new field and it is already contemplated in this behat test |
There was a problem hiding this comment.
@yeniatencio good work. Thanks a lot for making all those changes :)
https://digital-vic.atlassian.net/browse/SDPAP-7925
BE link: https://nginx-php.pr-323.content-reference-sdp-vic-gov-au.sdp4.sdp.vic.gov.au/