Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dev/core#1936 Make the label field on price_field_value non required … #18124

Merged
merged 1 commit into from
Aug 11, 2020

Conversation

seamuslee001
Copy link
Contributor

…field

Overview

5.27 port of #18123

Before

Label field required

After

label field non required

ping @KarinG @demeritcowboy @eileenmcnaughton

Karin in your testing you should update the upgrade function to be 5_27_4 but we need 5_27_5 for core because that will be the next available version

@civibot
Copy link

civibot bot commented Aug 11, 2020

(Standard links)

@civibot civibot bot added the 5.27 label Aug 11, 2020
@KarinG
Copy link
Contributor

KarinG commented Aug 11, 2020

I still can not upgrade -> I still get stuck on [Executed: Upgrade DB to 5.27.4: SQL]

[11-Aug-2020 10:40:48 America/Edmonton] Error: Call to undefined method CRM_Core_I18n::getMultilingual() in /Applications/MAMP/htdocs/d8civicrm.local/vendor/civicrm/civicrm-core/CRM/Upgrade/Incremental/php/FiveTwentySeven.php on line 92 #0 /Applications/MAMP/htdocs/d8civicrm.local/vendor/civicrm/civicrm-core/CRM/Queue/Task.php(74): CRM_Upgrade_Incremental_php_FiveTwentySeven::priceFieldValueLabelNonRequired(Object(CRM_Queue_TaskContext))

@KarinG
Copy link
Contributor

KarinG commented Aug 11, 2020

Re-ran it with xdebug:
image

@KarinG
Copy link
Contributor

KarinG commented Aug 11, 2020

Notes $locales are fetched differently for public function priceFieldValueLabelRequired($ctx) {

    $domain = new CRM_Core_DAO_Domain();
    $domain->find(TRUE);
    if ($domain->locales) {
      $locales = explode(CRM_Core_DAO::VALUE_SEPARATOR, $domain->locales);

@seamuslee001
Copy link
Contributor Author

thanks @demeritcowboy @KarinG fixed the upgrader now

@demeritcowboy
Copy link
Contributor

I think you need to explode/unserialize the locales as @KarinG notes, as in CRM_Core_I18n::getMultilingual()

@seamuslee001
Copy link
Contributor Author

thanks @demeritcowboy fixed now

@KarinG
Copy link
Contributor

KarinG commented Aug 11, 2020

Ok one more time! :-)

Installed 5.27.4 - vanilla - create Contribution Page -> reproducing the issue:
image
image

Ok then:

karins-MBP:civicrm-core sysadmin$ curl https://patch-diff.githubusercontent.com/raw/civicrm/civicrm-core/pull/18124.diff -o 18124.diff

karins-MBP:civicrm-core sysadmin$ cat 18124.diff |patch -p1
patching file CRM/Core/I18n/SchemaStructure.php
patching file CRM/Price/DAO/PriceFieldValue.php
patching file CRM/Upgrade/Incremental/php/FiveTwentySeven.php
patching file Civi/Api4/Service/Spec/Provider/PriceFieldValueCreationSpecProvider.php
patching file tests/phpunit/CRM/Price/BAO/PriceFieldValueTest.php
patching file xml/schema/Price/PriceFieldValue.xml
karins-MBP:civicrm-core sysadmin$

For testing the upgrade
set civicrm_domain.version = 5.27.3

emacs CRM/Upgrade/Incremental/php/FiveTwentySeven.php
change -> public function upgrade_5_27_5($rev) {
to: public function upgrade_5_27_4($rev) {

Ok here we go - running upgrade -> Done!

Checking:

This looks good:
image

and this looks good too:
image

@seamuslee001
Copy link
Contributor Author

going to add Merge on pass based on @KarinG 's testing on the linked PRs

@seamuslee001
Copy link
Contributor Author

Test fails unrelated

@seamuslee001 seamuslee001 merged commit b019a41 into civicrm:5.27 Aug 11, 2020
@seamuslee001 seamuslee001 deleted the dev_core_1936_527 branch August 11, 2020 23:26
@KarinG
Copy link
Contributor

KarinG commented Aug 11, 2020

Phew!

@eileenmcnaughton
Copy link
Contributor

Thanks @seamuslee001 - the approach had been agreed on chat & I agree @KarinG's testing was solid on this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants