User tests: Successful: Unsuccessful:
Status | New | ⇒ | Pending |
Labels |
Added:
?
?
|
I have tested this item 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>
Labels |
Added:
?
|
Milestone |
Added: |
I have tested this item successfully on b8f060a
Test OK
This PR has received new commits.
CC: @anibalsanchez, @photodude
@Kubik-Rubik does this really have to pushed back until 3.6? No chance to add it to 3.5?
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.
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.
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!
I agree, "It should be considered an improvement to an existing feature" and or a "bugfix"
I concur. It is an enhancement to simplify query definition.
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.
This shouldn't be considered new functionality when it is fixing a bug!
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.
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).
@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!
@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
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).
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.
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.
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).
@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
@photodude Please feel free. :-)
@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.
@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.
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).
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).
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.
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.
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.
Status | Pending | ⇒ | Ready to Commit |
Labels |
Category | ⇒ | Libraries |
Whichever version this goes in - it is RTC
Labels |
Added:
?
|
Looking forward to seeing this in the development of 3.6
Status | Ready to Commit | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-04-09 14:22:56 |
Closed_By | ⇒ | wilsonge |
Labels |
Removed:
?
|
Milestone |
Added: |
Milestone |
Removed: |
Milestone |
Added: |
Milestone |
Removed: |
Awesome I'll see if I can get around to testing soon.