Write a source plugin + tests for 6.x Field instances [#2121393]

Comment	File	Size	Author
#12	interdiff.txt	4.8 KB	chx
#8	imp-create_source_field_instance-2121393-8.patch	5.31 KB	fastangel
#4	imp-create_source_field_instance-2121393-4.patch	5.11 KB	fastangel
#1	imp-create_source_field_instance-2121393-1.patch	5.04 KB	fastangel

Comment #1

fastangel CreditAttribution: fastangel commented 28 October 2013 at 09:28

Status:

Active

» Needs review

File	Size
imp-create_source_field_instance-2121393-1.patch	5.04 KB

Created a first version and added a test with a field body (type field with multiple rows).

Log in or register to post comments

Comment #2

chx CreditAttribution: chx commented 30 October 2013 at 20:52

This is an excellent issue to use the new Drupal6SqlBase because the table name changes with the module version.

Log in or register to post comments

Comment #3

chx CreditAttribution: chx commented 30 October 2013 at 20:53

Status:

Needs review

» Needs work

fixing status.

Log in or register to post comments

Comment #4

fastangel CreditAttribution: fastangel commented 4 November 2013 at 19:28

Issue summary:	View changes
Status:	Needs work	» Needs review

File	Size
imp-create_source_field_instance-2121393-4.patch	5.11 KB

Updated.

Log in or register to post comments

Comment #5

chx CreditAttribution: chx commented 4 November 2013 at 22:26

Status:

Needs review

» Needs work

I am sorry, I have explained this poorly. If you check content.module, it uses content_field_tablename for retrieving the table name depending on the module schema version. Drupal6SqlBase has a method to check for the module schema and so you can query the proper table.

Log in or register to post comments

Comment #6

marvil07 CreditAttribution: marvil07 commented 5 November 2013 at 06:05

Actually I guess that function exists only because it is used on install file.
Even if that is not the case, I will suggest to just simply require version >= 6001(reviewing cck git history, that change was introduced before 6.x-1.0-alpha1, so it's safe to assume it's in) on RequirementsInterface::checkRequirements(). See Drupal6SqlBase::getModuleSchemaVersion() for that check.

Log in or register to post comments

Comment #7

marvil07 CreditAttribution: marvil07 commented 5 November 2013 at 06:06

1 file was hidden/shown/deleted

File	Size
imp-create_source_field_instance-2121393-1.patch	5.04 KB

Log in or register to post comments

Comment #8

fastangel CreditAttribution: fastangel commented 6 November 2013 at 08:42

Status:

Needs work

» Needs review

File	Size
imp-create_source_field_instance-2121393-8.patch	5.31 KB

Done the last changes commented.

By the way, what happen with version before 6.x-1.0-alpha1?. We exclude from migrate? Should we say in some comment?

Log in or register to post comments

Comment #9

chx CreditAttribution: chx commented 6 November 2013 at 09:18

We were talking of a requirements interface that pre migrations every plugin (source, destination) can throw an exception in and the runner will skip that migration.

Log in or register to post comments

Comment #10

chx CreditAttribution: chx commented 6 November 2013 at 11:31

So I'd guess we should put in a requirements function that checks and throws an exception and then later we can wire it in with the interface.

Log in or register to post comments

Comment #11

fastangel CreditAttribution: fastangel commented 6 November 2013 at 11:43

ok. Then should I add a exception when the version is version < 6001. Right?

The code should be:

 public function checkRequirements() {
    //Code the check schema version if is < 6001 launch a exception.
    return $this->moduleExists('content');
  }

Log in or register to post comments

Comment #12

chx CreditAttribution: chx commented 10 November 2013 at 05:15

Status:

Needs review

» Fixed

File	Size
interdiff.txt	4.8 KB

I have committed this but please see the attached interdiff to see how much I needed to change this. (I am not talking of the TestFieldInstance, that's somewhat new and you might've not heard of it.)

Log in or register to post comments

Comment #13

fastangel CreditAttribution: fastangel commented 10 November 2013 at 08:59

Thanks. The code are changing so fast. I know that I need to stay into the weekly hangout to stay update.

Log in or register to post comments

Comment #14

chx CreditAttribution: chx commented 10 November 2013 at 09:08

My problem is, as I mentioned in #12 is not the new code. The problem is you are submitting code that clearly never ever has been tried -- because it contains numerous syntax errors. You can not use any expression in the default value of a property to boot.

Log in or register to post comments

Comment #15

chx CreditAttribution: chx commented 10 November 2013 at 09:10

Do you need assistance with running phpunit tests?

try

cd core
vendor/bin/phpunit --group migrate

Log in or register to post comments

Comment #16

chx CreditAttribution: chx commented 17 November 2013 at 07:59

Issue summary:	View changes
Priority:	Normal	» Critical
Status:	Fixed	» Active

I am reopening this to review any variables that might need to be added in prepareRow.

Log in or register to post comments

Comment #17

fastangel CreditAttribution: fastangel commented 17 November 2013 at 20:05

Assigned:	Unassigned	» fastangel
Status:	Active	» Needs review

Any relevant variable to use. See grep in cck:

./content.install:  if (variable_get('content_update_1009', FALSE)) {
./content.module:  if (!defined('MAINTENANCE_MODE') && variable_get('content_schema_version', -1) >= 6007) {
./content.module:      if (variable_get('content_schema_version', -1) < 6007) {
./content.module:  if (variable_get('content_schema_version', -1) < 6007) {
./content.module:  if (variable_get('content_schema_version', -1) < 6007) {
./content.module:    if (variable_get('content_schema_version', -1) < 6007) {
./content.module:        foreach (variable_get('content_extra_weights_'. $type_name, array()) as $key => $value) {
./content.module:    $version = variable_get('content_schema_version', 0);
./content.module:    $version = variable_get('content_schema_version', 0);
./content.module:    $version = variable_get('content_schema_version', 0);
./content.module:  if (variable_get('content_schema_version', -1) < 6000) {
./content.module:  if (module_exists('locale') && variable_get("language_content_type_$type_name", 0)) {
./content.module:  if (module_exists('upload') && variable_get("upload_$type_name", TRUE)) {
./includes/content.crud.inc:    if ($extra = variable_get('content_extra_weights_'. $info->old_type, array())) {
./modules/content_copy/content_copy.module:  if ($extra = variable_get('content_extra_weights_'. $form_values['type_name'], array())) {
./modules/fieldgroup/fieldgroup.module:    $version = variable_get('fieldgroup_schema_version', 0);
./modules/fieldgroup/fieldgroup.module:    $version = variable_get('fieldgroup_schema_version', 0);

Log in or register to post comments

Comment #18

chx CreditAttribution: chx commented 18 November 2013 at 09:39

> 'content_extra_weights_'. $info->old_type

Looks suspicious. What is this used for? D7 core allowed reordering of the fields and stored that in variables too. Is this the same?

Log in or register to post comments

Comment #19

chx CreditAttribution: chx commented 18 November 2013 at 10:14

More on D7: field_ui_display_overview_form_submit calls field_bundle_settings which sets / gets the variable field_bundle_settings_' . $entity_type . '__' . $bundle and from field_ui_display_overview_form_submit it is plain the weight is stored in that variable:

    $bundle_settings['extra_fields']['display'][$name][$view_mode] = array(
      'weight' => $form_values['fields'][$name]['weight']

Under D8, I would recommend trying to install field ui play with it a little -- weights should be in the entity.display.$this->entity_type . '.' . $this->bundle . '.' . $mode file .

In short, if that weight is indeed the same as in D7 then it needs to be added to the instance and then we can deal with it later when we migrate instances into instances and display entities.

Log in or register to post comments

Comment #20

marvil07 CreditAttribution: marvil07 commented 19 November 2013 at 17:35

Status:

Needs review

» Needs work

Reflecting issue status

Log in or register to post comments

Comment #21

fastangel CreditAttribution: fastangel commented 24 November 2013 at 20:24

I was doing some test here. And yes the weight is the same. Then we need to check the variable with height and map. Let me work on this.

Log in or register to post comments

Comment #22

fastangel CreditAttribution: fastangel commented 24 November 2013 at 20:32

Uhmm I was checking again in D6. http://api.drupalize.me/api/drupal/function/content_field_overview_form/6 overview in D6 isn't use variable. The only variable is used for extra _weight with custom fields implemented with extra weights instead field api. Then I think that this issue should be postponed to D7. What do you think?

Log in or register to post comments

Comment #23

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 26 February 2014 at 11:50

Is this to cover all of CCK migrations? Or just the fields that have been built in D6 (like node body mentioned above) and made optional in D7/8? Hard to grok the scope from the summary / discussion.