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

Use stateless flag from toolkit.ini it is present to override value in $_options array. #39

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Scott-Cam
Copy link
Contributor

The stateless flag from the toolkit.ini should be used if it is present in the config file. This value should override the stateless flag set in the $_options array.

@alanseiden
Copy link
Collaborator

Looks good. To clarify for others reading this, $_options is an array in ToolkitService.php that contains default values at first. By default, stateless is false. Scott is providing a way to change the default to true via toolkit.ini.

@alanseiden
Copy link
Collaborator

Scott, one problem, I think. This solution will change the default for everyone, won't it? Because stateless=true by default in toolkit.ini. All the innocent stateful users will suddenly get stateless behavior. Don't get me wrong, I think stateless should have been the default from the start, but now people's applications may start behaving differently. Am I correct about this?

@Scott-Cam
Copy link
Contributor Author

Yes, I'll update the toolkit.ini to set it to false and put a note there that it should be updated unless statefull connections are required.

@alanseiden
Copy link
Collaborator

Agreed, but will a value of false affect CW users? Their default was always true (to be compatible with Easycom default behavior).

@Scott-Cam
Copy link
Contributor Author

Yes, it will. So I guess it comes down to (in)convenience :) , what
benefits the majority of the users? Should the default be statefull or
stateless? Do you have a feeling (or hard numbers) on which is used more?
I think that since this is going in a new release it could be either, as
long as it is well documented, release notes, readme, in the ini file.
Then it is up to the user to decide which they prefer (or need) and
should be tested in a development environment prior to upgrading for
existing users, and new users would have to have a specific use case to
need the flag to be flipped (if the default is changed to stateless).

Let me know what you think and I can set it either way.

Scott

On 2/22/2015 8:25 PM, Alan Seiden wrote:

Agreed, but will a value of false affect CW users? Their default was
always true (to be compatible with Easycom default behavior).


Reply to this email directly or view it on GitHub
#39 (comment).

@alanseiden
Copy link
Collaborator

Scott,

If we were starting over, I'd default to stateless. 50% of people's problems stem from people following the samples and thus using stateful+disconnect, or omitting any mention of stateless, thus using stateful mode without knowing it (with the dreaded default internal key /tmp/Toolkit). Stateless mode simplifies life for most users.

If we were building toolkit 2.0, we could make major changes, impair backward compatibility, and break people's code if sufficiently documented. Because we're only working on 1.6.* or 1.7 at best, however, we must not break anyone's code. In the history of my toolkit involvement, I introduced only one bc break and it was because I corrected a bug, and even then, I included bug-compatible code that could be invoked via new .ini settings (the two "integrity" settings). This is IBM i, where users expect stability above all. This does make our job harder.

I think the only answer is to add a new setting that applies to the "new" toolkit, leaving the old CW setting as is. Then the old one could be set to true and the new one to false, along with a comment educating users that "true" is easier for most users.

@alanseiden
Copy link
Collaborator

We need a new, additional stateless setting that doesn't interfere with the old CW stateless setting. I don't see a way around that without breaking people's code.

@Scott-Cam
Copy link
Contributor Author

Agreed. I'll take a look at adding it in.

Scott

On 3/6/2015 2:34 PM, Alan Seiden wrote:

We need a new, additional stateless setting that doesn't interfere
with the old CW stateless setting. I don't see a way around that
without breaking people's code.


Reply to this email directly or view it on GitHub
#39 (comment).

@adamculp
Copy link
Contributor

Scott,

Did you get a chance to look at adding this in? I wanted to clean out this PR and at the moment it cannot be merged in.

@Scott-Cam
Copy link
Contributor Author

Sorry was on vacation so took a while to get everything else done this week.

Added a new flag in the toolkit.ini and check if the call is being made from the compatibility wrapper or not to return the appropriate stateless flag. Did some testing (stepping through the code for various calls) to ensure the calls that were from the compatibility wrapper were using the stateless flag and those that were not were using the xmlstateless flag.

Take a look and let me know if you think that there are any further changes required.

@alanseiden
Copy link
Collaborator

Thanks, Scott. Will look!

@chukShirley
Copy link
Contributor

@alanseiden Is this ready to merge?

@alanseiden
Copy link
Collaborator

@chukShirley The idea seems good, but what do you think of the flag's name? 'xmlstateless'. The flag's purpose is to change the default value of 'stateless' to true, for the regular toolkit (not CW). I think we should not use the word 'xml', because in the future, we may have json or other formats, and people shouldn't think this feature has anything to do with XML. How about calling it 'stateless_mode_default' or something like that? We could merge, if the name could be a bit better. Any ideas?

@chukShirley
Copy link
Contributor

I like 'stateless_mode_default'. That seems more descriptive.

@chukShirley
Copy link
Contributor

@Scott-Cam Can you rebase this branch and change the name of the flag?

@adamculp
Copy link
Contributor

Will this break BC for those who may already be using the previous default?

@clarkphp
Copy link
Member

clarkphp commented Oct 27, 2016

@adamculp - This is a new flag which does not exist in the current Toolkit, although it does have a pre-existing, equivalent flag of a different name in the current toolkit. The two flags have the exact same purpose and functionality; one is set to a more proper default, while the other is left at the old, non-optimal-but-non-BC-breaking default.

The old one will read if using the CW, while the new one will be read if not using the CW. There should be no BC break because no one currently using the toolkit is using the new flag.

See lines 1753 through 1758 of ToolkitService.php in the pull request. (the name is changing to stateless_mode_default, though).

@adamculp
Copy link
Contributor

Thanks Clark. I did not realize that since a previous comment said it was a toolkit.ini setting. Made me think it existed there previously.

…de value in

$_options array.  Add to constructor by adding to the array in
getOptionalParams.

Change default stateless flag to flase in the toolkit.ini to match
existing functionality

Add second stateless flag for XML toolkit.  Check if call is being made from compatibility wrapper and use appropriate stateless flag.
@Scott-Cam
Copy link
Contributor Author

Done, hopefully got it right (first rebase), the pull request is
updated. Let me know if there are any issues or additional changes
required.

Scott

On 10/25/2016 10:37 AM, Chuk Shirley wrote:

@Scott-Cam https://github.com/Scott-Cam Can you rebase this branch
and change the name of the flag?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#39 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AJY6-nOAK-IxTSwcw69my7U_3Muj2S_gks5q3jBfgaJpZM4Dgwzn.

@@ -144,7 +145,7 @@ public function __construct($databaseNameOrResource, $userOrI5NamingFlag = '0',
}

// Optional params. Don't specify if not given in INI.
$this->getOptionalParams('system', array('v5r4', 'ccsidBefore', 'ccsidAfter', 'useHex', 'paseCcsid', 'trace', 'dataStructureIntegrity', 'arrayIntegrity'));
$this->getOptionalParams('system', array('stateless_mode_default','stateless','v5r4', 'ccsidBefore', 'ccsidAfter', 'useHex', 'paseCcsid', 'trace', 'dataStructureIntegrity', 'arrayIntegrity'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

@alanseiden Don't include 'stateless' here. That's only for the CW.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@alanseiden Around line 157, try something like:

if ($this->getConfigValue('system', 'stateless_mode_default')) {
            $this->serviceParams['stateless'] = $this->getConfigValue('system', 'stateless_mode_default');
        }

@@ -1734,7 +1735,12 @@ public function getInternalKey()
*/
public function isStateless()
{
return $this->getOption('stateless');
if ($this->getIsCw()){
Copy link
Collaborator

Choose a reason for hiding this comment

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

@alanseiden This isn't the place to make this change. 'stateless' from getOptions() is still the value to check.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alan, see above comment about line 157.

; Non-CW Stateless flag - default to true
; This should be overridden if you require stateful connections
; Set the default value of 'stateless' to true, for the regular toolkit (not CW).
stateless_mode_default = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

@alanseiden Should default to false to retain traditional behavior (don't break applications by mistake).

stateless = true
; CW stateless flag
; stateless mode is default for i5_connect (though automatically overridden if private conns are used)
stateless = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

@alanseiden stateless flag for CW should be true by default as it currently is.

; stateless mode is default for i5_connect (though automatically overridden if private conns are used)
stateless = false
; Non-CW Stateless flag - default to true
; This should be overridden if you require stateful connections
Copy link
Collaborator

Choose a reason for hiding this comment

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

@alanseiden Suggested comments instead:

; Non-CW (regular toolkit) Stateless default flag
; Set the default value of 'stateless' option, for the regular toolkit (not CW).

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.

5 participants