? ? ? Success

User tests: Successful: Unsuccessful:

avatar chrisdavenport
chrisdavenport
16 Dec 2015

Updates and replaces #5837

For full description and test instructions, see #5837

Votes

# of Users Experiencing Issue
1/1
Average Importance Score
5.00

avatar chrisdavenport chrisdavenport - open - 16 Dec 2015
avatar chrisdavenport chrisdavenport - change - 16 Dec 2015
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 16 Dec 2015
Labels Added: ? ?
avatar photodude
photodude - comment - 17 Dec 2015

Awesome :+1: I'll see if I can get around to testing soon.

avatar photodude photodude - test_item - 17 Dec 2015 - Tested successfully
avatar photodude
photodude - comment - 17 Dec 2015

I have tested this item :white_check_mark: successfully on b8f060a

Successful test for the orWhere() and extendWhere() methods as I was able to replace the following query

$query->where('( a.account_name LIKE ' . $search . ' OR a.account_number LIKE ' . $search . ' OR a.crm_id LIKE ' . $search . ' )');

With the following

$query->where('( a.account_name LIKE ' . $search . ' )')
      ->orWhere('( a.account_number LIKE ' . $search . ' )', 'XOR')
      ->orWhere('( a.crm_id LIKE ' . $search . ' )', 'XOR');

Resulting search filter had unchanged behavior (and is much easier to read and convey the meaning with the changes)

This change also worked with extendWhere()

$query->where('( a.account_name LIKE ' . $search . ' )')
    ->extendWhere('XOR','( a.account_number LIKE ' . $search . ' )')
    ->extendWhere('XOR','( a.crm_id LIKE ' . $search . ' )');
```<hr /><sub>This comment was created with the <a href="https://github.com/joomla/jissues">J!Tracker Application</a> at <a href="https://issues.joomla.org/tracker/joomla-cms/8718">issues.joomla.org/joomla-cms/8718</a>.</sub>
avatar Kubik-Rubik Kubik-Rubik - change - 17 Dec 2015
Labels Added: ?
avatar Kubik-Rubik Kubik-Rubik - change - 17 Dec 2015
Milestone Added:
avatar anibalsanchez anibalsanchez - test_item - 19 Dec 2015 - Tested successfully
avatar anibalsanchez
anibalsanchez - comment - 19 Dec 2015

I have tested this item :white_check_mark: successfully on b8f060a

Test OK


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8718.

avatar joomla-cms-bot
joomla-cms-bot - comment - 19 Dec 2015

This PR has received new commits.

CC: @anibalsanchez, @photodude


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8718.

avatar photodude
photodude - comment - 19 Dec 2015

@Kubik-Rubik does this really have to pushed back until 3.6? No chance to add it to 3.5?

avatar mbabker
mbabker - comment - 19 Dec 2015

Technically 3.5 is in beta phase which should mean feature complete and only bug fixes going forward. But hey, wouldn't be the first time something is done against the published dev strategy.

avatar photodude
photodude - comment - 19 Dec 2015

Considering that this previously had a 3.5 milestone tag (original release) but somehow fell through the human testing cracks over the last year and didn't get the attention it deserves. I would hope that maybe it could regain it's original intended release placement. But I can see your point @mbabker.

avatar sovainfo
sovainfo - comment - 19 Dec 2015

Apart from the fact that this doesn't deserve the New Feature label.
It should be considered an improvement to an existing feature!

Would even call this a bugfix considering the poor curren implementation of where!

avatar photodude
photodude - comment - 21 Dec 2015

I agree, "It should be considered an improvement to an existing feature" and or a "bugfix"

avatar anibalsanchez
anibalsanchez - comment - 21 Dec 2015

I concur. It is an enhancement to simplify query definition.

avatar mbabker
mbabker - comment - 21 Dec 2015

Someone needs to define what is considered a feature in the world of Joomla then. Semantic Versioning states:

Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backwards compatible functionality is introduced to the public API.

As this PR introduces new API endpoints, by that definition this change is a new feature. Given Joomla's supposed adoption of Semantic Versioning and a lack of any clarifying guidance in the published development strategy, that text tells me new endpoints should only be introduced at minor version bumps. Given 3.5 is already in beta phase, I'd consider it feature complete per the development strategy and new features must be targeted for 3.6 going forward.

Note I'm not arguing the feature. But frankly why bother publishing rules if they aren't going to be followed.

avatar sovainfo
sovainfo - comment - 21 Dec 2015

This shouldn't be considered new functionality when it is fixing a bug!

avatar chrisdavenport
chrisdavenport - comment - 21 Dec 2015

If it's a bug fix, then it must be a really trivial one as the original PR was waiting for testers since January. It's a new feature, albeit a small one. I would love to have seen it in 3.5, but it's too late now. It will go into 3.6.

avatar mbabker
mbabker - comment - 21 Dec 2015

You and I have vastly different definitions of fixing bugs then. If fixing
a bug requires new API end points, bug fix or not it introduces new
functionality. The same argument goes for bug fixes that require B/C
breaks. Everyone can agree the patch fixes a valid bug but if it
introduces a break it must be handled as such.

On Monday, December 21, 2015, sovainfo notifications@github.com wrote:

This shouldn't be considered new functionality when it is fixing a bug!


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

avatar sovainfo
sovainfo - comment - 21 Dec 2015

@chrisdavenport The amount of attention given to PR here is more matter of popularity than a matter of severity.
Considering the level of attention anything about the database gets here it is clear there is a lack of expertise writing database applications in this project.
No surprise there considering the amount of trouble you encounter trying to contribute something in that area.
Can't blame them for giving up and spend their time somewhere else!

avatar sovainfo
sovainfo - comment - 21 Dec 2015

@mbabker: It also says under number 6:
Patch version Z (x.y.Z | x > 0) MUST be incremented if only backwards compatible bug fixes are introduced. A bug fix is defined as an internal change that fixes incorrect behavior.

To me the above qualifies as
1. BUG fix
2 Backwards compatible

avatar mbabker
mbabker - comment - 21 Dec 2015

What bug is being fixed? I'll argue that as this patch doesn't replace any
existing lines of code, it's only new features and API end points. Yes
these end points are beneficial and add needed flexibility. Yes this
should be merged when tested. I'll even go so far as to say it'll be in
3.5 if tested soon because Joomla is very lenient when it comes to
following its own rules (the last two security releases should have been
4.0.0 and 5.0.0 respectively because of B/C breaks). But the way I read
this patch as presented, it is only introducing new API end points and must
be treated as a feature patch.

On Monday, December 21, 2015, sovainfo notifications@github.com wrote:

@mbabker https://github.com/mbabker: It also says under number 6:
Patch version Z (x.y.Z | x > 0) MUST be incremented if only backwards
compatible bug fixes are introduced. A bug fix is defined as an internal
change that fixes incorrect behavior.

To me the above qualifies as
1. BUG fix
2 Backwards compatible


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

avatar sovainfo
sovainfo - comment - 21 Dec 2015

Quote of @chrisdavenport in #5837:
The current implementation of the database query where method is limited by an unfortunate restriction in that the first use of the glue parameter fixes the glue for all subsequent uses.

Qualifies as a bug to me. And this pr implements a backwards compatiblie solution.
Agree that is not complete because it doesn't take away the bad code. But does provide the expected functionality. For the short term good enough, not expecting a rewrite of the database api in J3.

avatar photodude
photodude - comment - 22 Dec 2015

I'm going to side with @mbabker on this even though I would really like to see this in 3.5 (hopefully 3.6 won't be more than 6 months out).

Semantic Versioning question Answer for this PR
new API methods (aka endpoints) yes
new API functionality yes
fixes an existing issue/bug yes
improvement to an existing feature yes, but through a new method.
Backwards compatible no (it's new functionality, old code would need special adaptation or modification to benefit from this fix)
changes existing lines of code no
internal change that fixes incorrect behavior no, it introduces a new method to address the incorrect behavior

I'll agree it fails all the tests for being just a bug fix, it does pass as a minor version item.

avatar mbabker
mbabker - comment - 22 Dec 2015

So it would qualify as both a bug fix and new feature implementation then.
Typically it is recommended to treat it as the "higher" of the two options
in this case, so even though it is a bug fix it also adds new API end
points which should trigger a minor release. That's how I would handle it
in any library, extension, or versioned code I maintain. Version numbers
are cheap, the only time effective communication of their meaning for
SemVer following projects has an issue is when marketing value is placed in
them.

I use this same type of assessment on all versioned code, publicly shared
or internal to my company. A bug fix requiring a B/C break will always get
a major version bump unless something is implemented in the affected API to
mitigate it. Then it would be a minor version bump since the old behavior
would be deprecated most likely and the suggestion (and IIRC Joomla's dev
strategy says it) is adding deprecation notices triggers a minor release.

It's nothing against Chris or anyone's opinions. It's how I manage
versioned code in part based on SemVer and its recommendations on how
version numbers communicate scope of change. In the case of Joomla, its
augmented by the dev strategy which also gives definitions for different
development states. Given all that, based on 3.5 having reached its beta
phase, the documented strategies are why this patch shouldn't be added to
3.5. Again, nothing personal, just my interpretation of the current
policies backed by my experience in Joomla and outside it.

On Monday, December 21, 2015, sovainfo notifications@github.com wrote:

Quote of @chrisdavenport https://github.com/chrisdavenport in #5837
#5837:
The current implementation of the database query where method is limited
by an unfortunate restriction in that the first use of the glue parameter
fixes the glue for all subsequent uses.

Qualifies as a bug to me. And this pr implements a backwards compatiblie
solution.
Agree that is not complete because it doesn't take away the bad code. But
does provide the expected functionality. For the short term good enough,
not expecting a rewrite of the database api in J3.


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

avatar photodude
photodude - comment - 22 Dec 2015

@chrisdavenport It would be nice to see this get added the the Joomla framework too. Would you consider adding it there? I would be willing to copy your work and add it if you're ok with that.
https://github.com/joomla-framework/database
(file needing this change)
https://github.com/joomla-framework/database/blob/master/src/DatabaseQuery.php

avatar chrisdavenport
chrisdavenport - comment - 22 Dec 2015

@photodude Please feel free. :-)

avatar photodude
photodude - comment - 2 Jan 2016

@chrisdavenport Add extendWhere, orWhere and andWhere methods are now part of the Framework database package 1.3.0, It will be nice to see this feature finally get accepted into the CMS at 3.6.

avatar sovainfo
sovainfo - comment - 2 Jan 2016

@mbabker @photodude Sounds like http://semver.org/spec/v2.0.0.html is diverting too much from the industry standard https://en.wikipedia.org/wiki/Software_versioning.

Consider Joomla choosing SemVer a mistake, should have been Software Versioning (Sequence based identifiers) in my opinion.

avatar chrisdavenport
chrisdavenport - comment - 2 Jan 2016

It's marked for release in 3.6. If you want to use it earlier, you can easily apply the patch to your own systems, safe in the knowledge that upgrading will not break anything (unless this PR gets changed in the meantime).

avatar mbabker
mbabker - comment - 2 Jan 2016

should have been Software Versioning (Sequence based identifiers) in my opinion

SemVer is using a sequence based identifier system in that it sets guidelines for when each part of the identifier should be incremented. The way that wiki article reads, it basically says "let the author figure out what their version number scheme means and hope people follow along". SemVer at least gives folks a common standard to work from.

Truthfully, I think too much marketing value has been placed in version numbers and how often they're incremented. People are scared to see a minor version bump too frequently, people are scared to see a major version bump at all. If you really want to throw people for a loop, let's use the SVN revision (since GitHub supports repo access using SVN) as the version number going forward. It's the most straightforward sequence based identifier we can provide (and yes that's me being sarcastic).

avatar sovainfo
sovainfo - comment - 2 Jan 2016

Consider the current definition of SemVer more appropriate for the Framework or libraries with defined API's. The purpose for the system is to communicate the impact of a new release. A patch release only with bugfixes should be no problem to apply. Same thing for a minor release with some added functionality. Should trigger you to look into the new functionality to see whether you could use it.
And guess what, major changes require a major release, surprise surprise!

It is no surprise that with the track record of this project you need to treat any release as a major one and check everything before you apply it to your production site. No wonder that people still don't update.

avatar mbabker
mbabker - comment - 2 Jan 2016

So how do you communicate version numbering for a distributed application which is built on an internal framework with a (albeit poorly) defined API? Part of what the Joomla CMS version number conveys is applicable to the developers writing code; a feature introduced in 3.2 cannot be used in a code base supporting 2.5 so one has to have a means to check that to know to programmatically enable/disable features.

SemVer is a fair compromise for all parties. The problem isn't in SemVer itself; the problem is (and always has been) with those who are supposed to be enforcing it. How many development strategies have been published by Joomla in the last five years governing the CMS, Platform, and Framework, and how many are truly enforced in some form today? I'll be the first person to tell you, even though I highly encouraged for it to be merged, #7217 should have never been accepted until 4.0 because it very blatantly breaks B/C.

avatar sovainfo
sovainfo - comment - 2 Jan 2016

That only proves how ridiculous it is to try to force semver upon Joomla CMS. Also it shows how bad the current procedure is.

Concerning #7217 with Software Versioning applicable it would have been accepted for the next maintenance release. This assumes it is considered a bugfix, that by definition makes the break in BC irrelevant.
It is not really rocket science to figure out that you shouldn't fake the version. The faster and more of these rookie kind of mistakes are corrected, the faster we are releaved of these embarrassing PR's.
Hope that people report the extensions messing with the version, don't think anyone would want to use them.

Hopefully it also makes it to the Standard and Guidelines, so people know not only how to write contributions but also how they should be judged.

avatar brianteeman brianteeman - change - 12 Jan 2016
Status Pending Ready to Commit
Labels
avatar brianteeman brianteeman - change - 12 Jan 2016
Category Libraries
avatar brianteeman
brianteeman - comment - 12 Jan 2016

Whichever version this goes in - it is RTC


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8718.

avatar joomla-cms-bot joomla-cms-bot - change - 12 Jan 2016
Labels Added: ?
avatar photodude
photodude - comment - 24 Mar 2016

Looking forward to seeing this in the development of 3.6

avatar wilsonge wilsonge - reference | 9783a53 - 9 Apr 16
avatar wilsonge
wilsonge - comment - 9 Apr 2016

Merged with 9783a53 - thanks Chris :)

avatar wilsonge wilsonge - change - 9 Apr 2016
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2016-04-09 14:22:56
Closed_By wilsonge
avatar wilsonge wilsonge - close - 9 Apr 2016
avatar brianteeman brianteeman - change - 11 May 2016
Labels Removed: ?
avatar wilsonge wilsonge - change - 11 May 2016
Milestone Added:
avatar wilsonge wilsonge - change - 11 May 2016
Milestone Removed:
avatar wilsonge wilsonge - change - 21 May 2016
Milestone Added:
avatar wilsonge wilsonge - change - 21 May 2016
Milestone Removed:

Add a Comment

Login with GitHub to post a comment