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

add outgoing field config to the ApplyFieldSaveEvent #16168

Merged
merged 4 commits into from
Nov 22, 2024

Conversation

i-just
Copy link
Contributor

@i-just i-just commented Nov 20, 2024

Description

Added oldConfig to the ApplyFieldSaveEvent to make it easier to compare the changes applied to the field.

In addition to what the OP said:

  • when saving the field via the control panel
    • the event only gets triggered once;
    • the field is of a new type
    • the config is also for the new type;
  • when applying PC
    • the event gets triggered twice (when updating an existing field);
    • first time, the field is of the old type, but the config is of the new type;
    • the second time it’s triggered, both the field and the config are of the new type;

Regardless of how the field is changed, the Fields->applyFieldSave() method still has the field record, which hasn’t been updated yet, which means we still have access to the outgoing field’s config. Getting that config and passing it in the event should give an easier way to compare the changes being applied to the field.

Related issues

#16156

@i-just i-just requested a review from brandonkelly November 20, 2024 15:52
@lindseydiloreto
Copy link
Contributor

Thanks @i-just! 🙏

Just to clarify... would the oldConfig value include the field type? Or simply the field configuration data?

I'm slightly confused about the difference between field and config here, but I assume that the field is the entire FieldInterface, whereas config is simply the field's configuration (as an array).

Once this PR is accepted, I assume my event logic would change to something closer to this...

// If no field is provided, bail
if (!$field = $event->field) {
    return;
}

// If not a Google Maps Address field, bail
if (!($field instanceof \doublesecretagency\googlemaps\fields\AddressField)) {
    return;
}

// If no original field, bail
if (!$event->oldConfig) {
    return;
}

// If original field wasn't a Maps Map field, bail
if (!$event->oldConfig instanceof \ether\simplemap\fields\MapField) {
    return;
}

// ... migrate the field's data...

Am I on the right track?

Will this logic be executed consistently between the original save versus applying the PC change?

@i-just
Copy link
Contributor Author

i-just commented Nov 21, 2024

@lindseydiloreto, the config includes the incoming (new) field config data (array) and the oldConfig would include the outgoing field config data (array or null). One of the items in both of those arrays is the field’s type. Comparing old and new config should be consistent between the control panel save an applying PC changes.

As to your code, I’d tweak that to check the config type instead of the field type, so something along the lines of:

// If no field is provided, bail
if (!$field = $event->field) {
    return;
}

// If not a Google Maps Address field, bail
if ($event->config['type'] !== 'doublesecretagency\googlemaps\fields\AddressField') {
    return;
}

// If no original field, bail
if (!$event->oldConfig) {
    return;
}

// If original field wasn't a Maps Map field, bail
if ($event->oldConfig['type'] !== 'ether\simplemap\fields\MapField') {
    return;
}

// ... migrate the field's data...

I hope this helps!

@brandonkelly
Copy link
Member

Just refactored the PR a bit. Removed the $oldConfig property, and just made sure that $field is always set to the field as it used to be configured.

@brandonkelly brandonkelly merged commit e69fdf8 into 4.x Nov 22, 2024
3 checks passed
@brandonkelly brandonkelly deleted the bugfix/16156-apply-field-save-event-add-old-config branch November 22, 2024 20:05
@lindseydiloreto
Copy link
Contributor

Just tested this, and it seems to be working perfectly! Thanks @i-just and @brandonkelly! 🎉 🙏

Here's the final code we're using to determine whether or not to perform the migration...

// If no field is provided, bail
if (!$field = $event->field) {
    return;
}

// If not currently an Ether "Map" field, bail
if (!($field instanceof \ether\simplemap\fields\MapField)) {
    return;
}

// If no new field config, bail
if (!$newConfig = $event->config) {
    return;
}

// If new config isn't an "Address (Google Maps)" field, bail
if ($newConfig['type'] !== AddressField::class) {
    return;
}

// ... migrate the field's data...

I believe it is now behaving consistently between CP fieldtype changes & Project Config fieldtype changes.

Thanks again!

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

Successfully merging this pull request may close these issues.

3 participants