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

Fix 10115 - Wokflow Calculate Action broken under PHP8 #10122

Merged
merged 1 commit into from Oct 30, 2023

Conversation

markbond1007
Copy link
Contributor

Added a check for when request id is set but set to blank.

Description

Currently when trying to set up a workflow with a calculate field action under PHP the form doesnt not generate,

  1. Create a new workflow
  2. Choose a module
  3. Add an Action
  4. Choose Calculate Field
  5. The form to fill in does not appear when using PHP8

Motivation and Context

Fixes broken code under PHP8

How To Test This

  1. Create a new workflow
  2. Choose a module
  3. Add an Action
  4. Choose Calculate Field
  5. The form to fill will appear after the fix is applied

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Final checklist

  • My code follows the code style of this project found here.
  • My change requires a change to the documentation.
  • I have read the How to Contribute guidelines.

@SuiteBot
Copy link

SuiteBot commented Jul 21, 2023

CLA assistant check
All committers have signed the CLA.

@chris001
Copy link
Contributor

@markbond1007 Shouldn't the branch be set to hotfix, not hotfix-7.13.x? And all 64 of your commits squashed to one commit?

@pgorod
Copy link
Contributor

pgorod commented Jul 23, 2023

Those 64 commits aren't his... he just needs to bring his branch up-to-date with the commits in the main repo, so that they don't all show inside this PR

@markbond1007
Copy link
Contributor Author

@pgorod

I directly forked this from master, made the update and pull requested it back to the hotfix 7.13.x as per the instructions, when looking at my fork it tells me i am 'ahead' of SuiteCRM:hotfix, I dont know why its comparing to hotfix as I forked master and pulled req'd to hotfix-7.13.x

@chris001
Copy link
Contributor

@markbond1007
Try and change this PR's branch to merge into, from hotfix-7.13.x to hotfix only. See if that clears this up.

@pgorod
Copy link
Contributor

pgorod commented Jul 24, 2023

I think what should be done (and I believe the instructions say this) is to start your branch from the branch you will be directing your PR towards. And make sure it is up to date before you start adding your own commits.

So typically this is what I would do:

  • I have my commits somewhere where I am working on this
  • I finish my work
  • I go and bring my SuiteCRM up to date with SalesAgility's, specifically the branch (like hotfix-7.13) where I aim to add my stuff
  • after it is up to date, I start a new branch from there (not from master)
  • I move my commits on top of that (just by editing the files, or using git stash, or using cherry-pick from another branch
  • I create the pull request

@markbond1007
Copy link
Contributor Author

LOL, well the docs I find today are definately different to those I followed a couple of days back (cant even find them now).

I dont do enough forking to follow that exactly, so I just followed the instructions on Github (salesagilities/Suite instructions), I cant find them now but I notice the ones I find now are pointed at the SuiteCRM docs site,

@pgorod thats basically what I did, I forked a brand new repo, made changes and the pull request'd it back (with steps inbetween about creating branches based on bug numbers).

The issue was the instructions I found said to fork from master and then go back to hotfix 7.13.x, the instructions say to send it back to hotfix (on SuiteCRM website).

@markbond1007
Copy link
Contributor Author

Well what I found was a version of this:

https://github.com/salesagility/SuiteCRM/blob/master/.github/CONTRIBUTING.md

@pgorod
Copy link
Contributor

pgorod commented Jul 26, 2023

I think that is quite out-dated. Any way - you fork an entire repository, not a branch. But then you start a new branch from a previously existing branch, and for the SuiteCRM project that's never going to be master. Always one of the hotfix* branches.

@markbond1007
Copy link
Contributor Author

markbond1007 commented Jul 26, 2023

@pgorod
Yes well I knew what I meant with forking branches ;-) the key is the document I found was waaaay out of date... (and last editted by you :D )

@jack7anderson7 jack7anderson7 changed the base branch from hotfix-7.13.x to hotfix August 7, 2023 14:52
… PHP8

Added a check for a set request id but set to blank.
@serhiisamko091184 serhiisamko091184 added Status: Requires Code Review Needs the core team to code review Status:Assessed PRs that have been tested and confirmed to resolve an issue by a core team member PR 4-8 Score given to PRs once assessed labels Aug 7, 2023
@serhiisamko091184
Copy link
Contributor

Hello @markbond1007 ,
thanks a lot for your contribution to SuiteCRM project!

Regards,
Serhii

@jack7anderson7 jack7anderson7 added the Area: Workflow Issues & PRs related to all things regarding workflow label Aug 30, 2023
@clemente-raposo clemente-raposo added Status: Passed Code Review Mark issue has passed code review reviewed Status: Requires Testing Requires Manual Testing and removed Status: Requires Code Review Needs the core team to code review labels Oct 9, 2023
@johnM2401 johnM2401 added Status: Passed Testing and removed Status: Requires Testing Requires Manual Testing labels Oct 12, 2023
@jack7anderson7 jack7anderson7 merged commit 13f5b57 into salesagility:hotfix Oct 30, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Workflow Issues & PRs related to all things regarding workflow PR 4-8 Score given to PRs once assessed Status:Assessed PRs that have been tested and confirmed to resolve an issue by a core team member Status: Passed Code Review Mark issue has passed code review reviewed Status: Passed Testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants