forked from civicrm/civicrm-core
-
Notifications
You must be signed in to change notification settings - Fork 4
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
PCHR-4346: Fix custom Home URL parsing #28
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Included in CiviCRM 5.8.0 Core PR: civicrm#13031
davialexandre
added a commit
to compucorp/civihr
that referenced
this pull request
Nov 1, 2018
The only reason we needed the overridden file was to fix an error in CiviCRM Core. Since this has been now fixed in core, we don't need to file anymore. References: civicrm/civicrm-core#13031 compucorp/civicrm-core#28
mickadoo
approved these changes
Nov 1, 2018
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 😎
davialexandre
added a commit
to compucorp/civihr
that referenced
this pull request
Nov 6, 2018
The only reason we needed the overridden file was to fix an error in CiviCRM Core. Since this has been now fixed in core, we don't need to file anymore. References: civicrm/civicrm-core#13031 compucorp/civicrm-core#28
vinuvarshith
pushed a commit
that referenced
this pull request
Sep 23, 2019
…QL 5.5 Overview -------- This disables a particularly flaky test-scenario. Technical Details ----------------- The `SyntaxConformanceTest::testSqlOperators()` has been rather flaky for a while. (I think this goes back either to the creation of the test or the expansion of Dedupe API -- in other words, the test of this scenario has always been flaky.) When the test fails, it looks like this: ``` api_v3_SyntaxConformanceTest::testSqlOperators with data set #28 ('Dedupe') incorrect count returned from Dedupe getcount Failed asserting that 0 matches expected 2. /home/jenkins/bknix-min/build/build-1/web/sites/all/modules/civicrm/Civi/Test/Api3TestTrait.php:191 /home/jenkins/bknix-min/build/build-1/web/sites/all/modules/civicrm/tests/phpunit/api/v3/SyntaxConformanceTest.php:1131 /home/jenkins/bknix-min/build/build-1/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php:215 /home/jenkins/bknix-min/civicrm-buildkit/extern/phpunit6/phpunit6.phar:570 ``` I've spent a chunk of time investigating the flakiness and found: 1. The test usually (possibly always?) passes on MySQL 5.7. 2. The test often (but not always) fails on MySQL 5.5. 3. The version of PHP (php70 vs php72) does not make much difference. 4. The test never seems to fail when run by itself. The smallest scenario which seemd to reliably produce the failure was: ``` env SYNTAX_CONFORMANCE_ENTITIES='ContactType Country Dedupe Domain StateProvince' \ CIVICRM_UF=UnitTests \ phpunit5 \ --stop-on-failure --tap \ tests/phpunit/api/v3/SyntaxConformanceTest.php \ --filter '(testCustomDataGet|testLimit|testSqlOperators)' ``` 5. The reproducibility differed on two laptops. The laptops were largely identical wrt PHP/MySQL binaries+configuration (macOS, bknix@master-loco, `min` cfg). They differed in OS version and hardware specs. The slower machine (low-wattage 2-core CPU; 8gb; Mojave) produced test-failures much more reliably than the faster machine (high-wattage 4-core CPU; 16gb; Sierra). Rationalizations for why it's OK to sip: * The test will continue to be monitored on MySQL 5.7+. * The test *does* pass reliably when run by itself on MySQL 5.5. * [Civi doesn't officially support MySQL 5.5](https://docs.civicrm.org/sysadmin/en/latest/requirements/#mysql-version), so we're not required to faff-about over a failure that only occurs on MySQL 5.5. See also: https://chat.civicrm.org/civicrm/pl/s7nbkro8yjfgzk5z4epo8g9jph
reneolivo
pushed a commit
that referenced
this pull request
Jul 2, 2020
Failure is due to 'Undefined class constant 'POST_SELECT_QUERY' see https://lab.civicrm.org/dev/joomla/-/issues/28
reneolivo
pushed a commit
that referenced
this pull request
Jul 2, 2020
…tall dev/joomla/#28 5.27 Upgrade fails
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
The
CRM/Core/BAO/Navigation.php
has been overridden in CiviHR to fix an E_NOTICE thrown while parsing the custom URL we use for the Home page (see compucorp/civihr#924).In the past, besides this error fix, there were other customizations added to this file, but after some time they have been removed and this was the last one remaining, as it can be seenn in this diff: https://www.diffchecker.com/kQra6Xsf.
Since this is an error in core, I've submitted a PR to fix it there so that we can remove the overridden file.
Before
The
Navigation.php
file was overridden in CiviHR to fix an error in CiviCRM core.After
The error has been fixed in Core and the overridden file isn't necessary anymore.
Comments
This PR is targeting the
5.7.0-beta1-patches
branch because the work to upgrade CiviHR to the latest CiviCRM version is still in progress and 5.7.0 has not been released yet (according to the CiviCRM release scheduled, it should be out on the first Wednesday of November). After the new version is out, a new5.7.0-patches
branch will be created and it will include this patch.For reference, this is the PR which removes the file from CiviHR: compucorp/civihr#2899