Title says it all. We should remove this constant and replace with an '8.x' string. This is not hard to maintain and messes with unit tests.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

Status: Needs review » Needs work

The last submitted patch, vdc.constants-bootstrap.patch, failed testing.

damiankloip’s picture

Component: views.module » base system
damiankloip’s picture

Title: Convert DRUPAL_CORE_COMPATIBILITY and VERSION constants in bootstrap.inc to Drupal class constants » Remove usage of DRUPAL_CORE_COMPATIBILITY constant in Drupal\views\Plugin\Core\Entity\View
Component: base system » views.module
Status: Needs work » Needs review
FileSize
562 bytes

Forget it, Let's move it back to just views.

damiankloip’s picture

Issue summary: View changes

Updated issue summary.

dawehner’s picture

+++ b/core/includes/bootstrap.inc
@@ -31,12 +31,12 @@
-const VERSION = '8.0-dev';
+const VERSION = Drupal::VERSION;
...
-const DRUPAL_CORE_COMPATIBILITY = '8.x';
+const DRUPAL_CORE_COMPATIBILITY = Drupal::CORE_COMPATIBILITY;

What about just removing that change ... then there is no installer problem.

damiankloip’s picture

FileSize
1007 bytes

ok, and just do this?

Status: Needs review » Needs work

The last submitted patch, 2067017-6.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
349 bytes
1000 bytes

Oops.

Status: Needs review » Needs work
Issue tags: -VDC

The last submitted patch, 2067017-8.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
Issue tags: +VDC

#8: 2067017-8.patch queued for re-testing.

jibran’s picture

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

How many instances of the other constants do exist? Maybe we should ask someone whether they should be converted as well. Is there a tag for it?

webchick’s picture

Hm. Why only remove it from this one file? If we're moving it to the Drupal class, we ought to update all instances everywhere, no? Else we risk updating it in one place and not the other.

webchick’s picture

Status: Reviewed & tested by the community » Needs review
damiankloip’s picture

Title: Remove usage of DRUPAL_CORE_COMPATIBILITY constant in Drupal\views\Plugin\Core\Entity\View » Remove usage of DRUPAL_CORE_COMPATIBILITY constant in classes
FileSize
7.42 KB
8.39 KB

ok, fair enough. How about this?

damiankloip’s picture

Title: Remove usage of DRUPAL_CORE_COMPATIBILITY constant in classes » Remove usage of DRUPAL_CORE_COMPATIBILITY/VERSION constant in classes
FileSize
6.96 KB
15.36 KB

Oh, and VERSION too as I've added that to Drupal too?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I think this is good!

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

Are we sure that we want to maintain the constants in the Drupal class and bootstrap.inc - this seems are bit weird.

Is it not possible just to switch everything to use the class constants?

damiankloip’s picture

Well, I think some constants are used before the class loader is initialized, so I don't think so...

If this is going to be an issue, I'm totally fine with just using the patch in #4, so at least I am happier for views tests. But it seems like other classes might as well get fixed along the way too. Plus, come on, that is not alot of code to maintain ;)

damiankloip’s picture

FileSize
41.43 KB
55.66 KB

OK, spoke to catch on IRC - We agreed that really there is no need for the current constants to exist, and their usage is pretty much limited to core anyway. So we can remove VERSION and DRUPAL_CORE_COMPATIBILITY and use the Drupal::* alterantives..

andypost’s picture

+++ b/core/modules/views/tests/Drupal/views/Tests/Plugin/Block/ViewsBlockTest.php
@@ -12,9 +12,6 @@
 // @todo Remove this once the constant got converted.
-if (!defined('DRUPAL_CORE_COMPATIBILITY')) {
-  define('DRUPAL_CORE_COMPATIBILITY', '8.x');
-}

is not @todo for this constant?

damiankloip’s picture

There are 2 other constant declarations below this one.

damiankloip’s picture

Title: Remove usage of DRUPAL_CORE_COMPATIBILITY/VERSION constant in classes » Remove usage of DRUPAL_CORE_COMPATIBILITY/VERSION constants
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

So, this is pretty solid.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

webchick’s picture

Title: Remove usage of DRUPAL_CORE_COMPATIBILITY/VERSION constants » Change notice: Remove usage of DRUPAL_CORE_COMPATIBILITY/VERSION constants
Priority: Normal » Critical
Status: Fixed » Active
Issue tags: +Needs change record
damiankloip’s picture

Status: Active » Needs review
Gábor Hojtsy’s picture

Retitled change notice from "have been removed" to "have moved". They are not removed, you can find them at a different place.

damiankloip’s picture

Fair enough.

webchick’s picture

Title: Change notice: Remove usage of DRUPAL_CORE_COMPATIBILITY/VERSION constants » Remove usage of DRUPAL_CORE_COMPATIBILITY/VERSION constants
Priority: Critical » Normal
Status: Needs review » Fixed
Issue tags: -Needs change record
alexpott’s picture

So this has broken Drush - see https://github.com/drush-ops/drush/pull/65

greg.1.anderson’s picture

The problem isn't so much that Drush has broken; Drush can change. The problem is that the fix is awkward for Drush.

See: https://github.com/drush-ops/drush/pull/65

Could we perhaps put the version in composer.json, as David Reid suggested? This is a standard attribute that's widely used; seems like it would be very valuable for Drupal to maintain its version here as well, for constancy. Side benefit: it would be good for Drush.

greg.1.anderson’s picture

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.