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

Support [if] shortcode fields recursively (magic-tags) #5471

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

JoryHogeveen
Copy link
Member

@JoryHogeveen JoryHogeveen commented Sep 17, 2019

Fixes: #5470

Description

Add support for recursive if shortcodes for relationship fields. Example:

  • [if relationship.relationship_field] content [/if]
  • [if relationship.sub_relationship.sub_relationship_field] content [/if]

TODO

  • [if] (confirmed on Slack by Adam Spelbring): 2021-04-06)
  • [each]

ChangeLog

  • Enhancement: Support [if] and [each] shortcode tags recursively (magic-tags)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.
  • My code includes automated tests for PHP and/or JS (if applicable).

@JoryHogeveen JoryHogeveen added Status: Need Dev Feedback Waiting on feedback from developer of PR Status: PR > QA pending QA needs to be done labels Sep 17, 2019
@JoryHogeveen JoryHogeveen added this to the Pods 2.7.16 milestone Sep 17, 2019
@JoryHogeveen JoryHogeveen self-assigned this Sep 17, 2019
@JoryHogeveen JoryHogeveen added the Status: In Progress Issue or PR is currently in progress but not yet done label Sep 17, 2019
@JoryHogeveen
Copy link
Member Author

@jimtrue @sc0ttkclark
Also see issue #5470

This PR will need a thorough testing round with all kings of shortcode and template usages (widgets??)
I'm not sure about the current extend of possibilities with this area of Pods so I can imagine I'm overlooking some features that need testing as well.

@sc0ttkclark especially this part: https://github.com/pods-framework/pods/pull/5471/files#diff-ab22a13affdb28c998776e73a1808abfR579-R585

@JoryHogeveen JoryHogeveen added the Status: PR > Pending Code Review PR is pending code review by core developers label Oct 24, 2019
sc0ttkclark
sc0ttkclark previously approved these changes Oct 25, 2019
@sc0ttkclark sc0ttkclark added Status: PR > Reviewed and Ready PR has been code reviewed by core developers and is ready for merge (if it passes QA) and removed Status: PR > Pending Code Review PR is pending code review by core developers Status: Need Dev Feedback Waiting on feedback from developer of PR labels Oct 25, 2019
Copy link

@tr1b0t tr1b0t left a comment

Choose a reason for hiding this comment

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

🚫 8 error(s)
⚠️ 2 warning(s)

Copy link

@tr1b0t tr1b0t left a comment

Choose a reason for hiding this comment

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

🚫 4 error(s)

@sc0ttkclark sc0ttkclark modified the milestones: Pods 2.7.16, Pods 2.7.17 Nov 12, 2019
@JoryHogeveen JoryHogeveen added Status: PR > QA pass QA passed and removed Status: PR > QA pending QA needs to be done labels Apr 19, 2021
@JoryHogeveen JoryHogeveen marked this pull request as ready for review April 19, 2021 17:23
@sc0ttkclark sc0ttkclark modified the milestones: Pods 2.7.28, Pods 2.7.29 May 19, 2021

$field_type = $pod->fields( $atts['field'], 'type' );
/**
Copy link
Member

Choose a reason for hiding this comment

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

Were the changes at https://github.com/pods-framework/pods/pull/5471/files#diff-3af4ca77fc3cd86765f464bb6fa7000725503257e80ccc885464b3ad43ab36c4L536-L540 not enough to do what you wanted here, is this part of the code still necessary? It seems that the previous field() alone would do what this does

Copy link
Member Author

Choose a reason for hiding this comment

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

No, currently only using the field method isn't enough. The field type and Pod object needs to be updated as well so traversals will work properly.
However, I do agree that this might not be the best way to do this.

@sc0ttkclark sc0ttkclark added Status: PR > Pending Code Review PR is pending code review by core developers and removed Status: PR > Reviewed and Ready PR has been code reviewed by core developers and is ready for merge (if it passes QA) labels May 19, 2021
Copy link
Member

@sc0ttkclark sc0ttkclark left a comment

Choose a reason for hiding this comment

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

Needs more discussion on the field() workaround

@sc0ttkclark sc0ttkclark modified the milestones: Pods 2.7.29, Pods 2.8 Aug 4, 2021
@sc0ttkclark
Copy link
Member

I recommend punting this and adding fields() support for traversal of relationship fields so that logic can be simplified

@sc0ttkclark
Copy link
Member

Traversal is in Whatsit::get_field() right now, what else do we need to do here to leverage that?

@sc0ttkclark sc0ttkclark modified the milestones: Pods 2.9, Pods 3.0 Jul 25, 2022
@sc0ttkclark sc0ttkclark marked this pull request as draft July 25, 2022 14:45
@JoryHogeveen
Copy link
Member Author

@sc0ttkclark Not sure, if you say traversal is already there then maybe this PR isn't needed anymore, or can at least get a big refactor.
It's been a long time since I worked on this, I'll need to revisit what I've done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Keyword: Needs Unit Tests Keyword: Puntable Status: PR > Pending Code Review PR is pending code review by core developers Status: PR > QA pass QA passed Type: Clean up / refactor Type: Enhancement Enhancements to features that already exist, but are not major additions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow recursive [if] and [else] statements
3 participants