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

PCHR-4346: Fix custom Home URL parsing #28

Merged
merged 1 commit into from
Nov 1, 2018

Conversation

davialexandre
Copy link
Member

@davialexandre davialexandre commented Nov 1, 2018

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 new 5.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

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
Copy link

@mickadoo mickadoo left a comment

Choose a reason for hiding this comment

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

Looks good 😎

@davialexandre davialexandre merged commit 5e201a9 into 5.7.beta1-patches Nov 1, 2018
@davialexandre davialexandre deleted the PCHR-4346-civicrm-upgrade branch November 1, 2018 12:45
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
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.

2 participants