Skip to content

Conversation

@lukasbestle
Copy link
Member

@lukasbestle lukasbestle commented Feb 12, 2023

This PR …

Features

  • Block and layout objects for the blocks and layout fields now allow access to the parent field object with $block->field(), $layout->field() and $layoutColumn->field().

Breaking changes

None

Ready?

  • Unit tests for fixed bug/feature
  • In-code documentation (wherever needed)
  • Tests and checks all pass

For review team

@lukasbestle lukasbestle added this to the 3.9.2 milestone Feb 12, 2023
@lukasbestle lukasbestle requested review from a team and distantnative February 12, 2023 18:52
@lukasbestle lukasbestle self-assigned this Feb 12, 2023
Copy link
Member

@distantnative distantnative left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, one small comment

@lukasbestle lukasbestle requested review from a team and distantnative February 13, 2023 16:36
distantnative
distantnative previously approved these changes Feb 13, 2023
@afbora
Copy link
Member

afbora commented Feb 13, 2023

@lukasbestle
Copy link
Member Author

lukasbestle commented Feb 13, 2023

@afbora Those are part of the reason why the $field prop is optional (the other reason being that it avoids the breaking change). I don't think we can get to the field object easily from these places.

Edit: Forget what I wrote, I think it should actually be doable. Thanks for spotting! Do you have time to update the code? Otherwise let's move this to 3.9.3.

@afbora
Copy link
Member

afbora commented Feb 13, 2023

Do you have time to update the code?

Actually this is easy for first one but other two ones are a little complicated since that ones inherit Form\FieldClass not Cms\Field.

Possible solutions in my mind:

  1. Support FieldClass class: protected Field|FieldClass|null $field;
  2. Add new field prop/method that returns new Field object to FieldClass class.

Which way do we should follow?

@lukasbestle
Copy link
Member Author

The goal of the new field prop is to make UUIDs for blocks possible in a future version. So I think we can't make it a form field, it needs to be a content field.

afbora
afbora previously approved these changes Feb 14, 2023
Copy link
Member

@afbora afbora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added only for first one (content field) 👍

@afbora
Copy link
Member

afbora commented Feb 14, 2023

I've created a PR that fixes CI workflow issue #5055

@lukasbestle
Copy link
Member Author

lukasbestle commented Feb 14, 2023

@afbora Thanks for updating the Layouts code and force-pushing the branch again.

I checked the other two uses in LayoutField again: I don't think we'll need to update those anyway as these calls won't be relevant for block UUIDs. Or if they are, we could still update them when we get to it. Maybe we'll have refactored the field setup anyway by then.

@lukasbestle
Copy link
Member Author

So looks good to me. Maybe @distantnative can have a final look before we merge this.

@afbora afbora requested a review from distantnative February 16, 2023 10:02
@lukasbestle lukasbestle merged commit 10aa71b into develop Feb 18, 2023
@lukasbestle lukasbestle deleted the feature/items-field-prop branch February 18, 2023 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants